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/ 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. regards, dan carpenter
