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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Fri, Jun 09, 2023 at 07:04:17PM -0400, Xin Long wrote:
> On Fri, Jun 9, 2023 at 12:41 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > On Fri, Jun 09, 2023 at 11:13:03AM -0400, Xin Long wrote:
> > 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/*.

Linus has been very clear that the BUG() in ping_err() is wrong and
should be removed.  But to me if you're very very sure a BUG() can't be
triggered that's more like a style or philosophy debate than a real life

When you look at ping_err() then it's like.  Ugh...  If we leave off the
else statement then GCC and other static checkers will complain that the
variables are uninitialized.  It we add a return then it communicates to
the reader that this path is possible.  But the BUG() silences the
static checker warning and communicates that the path is impossible.

A different solution might be to do a WARN(); followed by a return.  Or
unreachable();.  But the last time I proposed using unreachable() for
annotating impossible paths it lead to link errors and I haven't had
time to investigate.  Another idea is that we could create a WARN() that
included an unreachable() annotation.

	} else {
		IMPOSSIBLE("An impossible thing has occured");

As a static analysis developer, I have made Smatch ignore WARN()
information because warnings happen regularly and the information they
provide is not useful.  Smatch does consider unreachable() annotations
as accurate.

Anyway, in this patch the situation is completely different.  Returning
wrong error codes is a very common bug.  It's already happened once and
it will likely happen again.

My main worry with this patch is that the networking maintainers will
say, "Thanks, but please delete all the calls to BUG() in this function".
I just selected this one because it was particularly bad and it needs to
be handled a bit specially.  Deleting all the other calls to BUG() isn't
something that I want to take on.

dan carpenter

[Index of Archives]     [Linux Networking Development]     [Linux OMAP]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     SCTP

  Powered by Linux