On Sat, Jun 10, 2023 at 2:28 AM Dan Carpenter [off-list ref] wrote:

On Fri, Jun 09, 2023 at 07:04:17PM -0400, Xin Long wrote:

quoted

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

quoted

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
issue.

https://lore.kernel.org/all/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@mail.gmail.com/ (local)

I see, didn't know that there are so many BUG() uses that are historic
in the kernel, including net/*.

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.

Got it, thanks for the extra information.
A WARN() with unreachable() annotation sounds like a good idea.

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.

Yeah, we should gradually replace these bogus BUG()s.

Anyway, for these two patches:
Acked-by: Xin Long <lucien.xin@gmail.com>

Thanks.