On Sat, Jun 10, 2023 at 2:28 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > 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 > issue. > > https://lore.kernel.org/all/CAHk-=wg40EAZofO16Eviaj7mfqDhZ2gVEbvfsMf6gYzspRjYvw@xxxxxxxxxxxxxx/ 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@xxxxxxxxx> Thanks.
