Re: [PATCH 2/2 net] sctp: fix an error code in sctp_sf_eat_auth()

On Fri, Jun 9, 2023 at 12:41 PM Dan Carpenter [off-list ref] wrote:

On Fri, Jun 09, 2023 at 11:13:03AM -0400, Xin Long wrote:

quoted

This one looks good to me.

But for the patch 1/2 (somehow it doesn't show up in my mailbox):

  default:
  pr_err("impossible disposition %d in state %d, event_type %d, event_id %d\n",
        status, state, event_type, subtype.chunk);
- BUG();
+ error = status;
+ if (error >= 0)
+ error = -EINVAL;
+ WARN_ON_ONCE(1);

I think from the sctp_do_sm() perspective, it expects the state_fn
status only from
enum sctp_disposition. It is a BUG to receive any other values and
must be fixed,
as you did in 2/2. It does the same thing as other functions in SCTP code, like
sctp_sf_eat_data_*(), sctp_retransmit() etc.

It is a bug, sure.  And after my patch is applied it will still trigger
a stack trace.  But we should only call the actual BUG() function
in order to prevent filesystem corruption or a privilege escalation or
something along those lines.

Hi, Dan,

Sorry, I'm not sure about this.

Look at the places where it's using  BUG(), it's not exactly the case, like
in ping_err() or ping_common_sendmsg(), BUG() are used more for
unexpected cases, which don't cause any filesystem corruption or a
privilege escalation.

You may also check more others under net/*.

Calling BUG() makes the system unusable so it makes bugs harder to
debug.  This is even mentioned in checkpatch.pl "Do not crash the kernel
unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery
code (if feasible) instead of BUG() or variants".

"absolutely unavoidable", I think in a module, if it gets a case that is not
expected at all, and the consequence (it may cause or has caused) is
unsure, WARN_ON_ONCE() is not enough.

Thanks.