Re: [PATCH] xfs: delete some dead code in xfile_create()

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


On Tue, Sep 12, 2023 at 09:23:15AM -0700, Darrick J. Wong wrote:
> On Tue, Sep 12, 2023 at 10:41:53AM -0500, Alex Elder wrote:
> > On 9/12/23 10:38 AM, Darrick J. Wong wrote:
> > > On Tue, Sep 12, 2023 at 06:18:45PM +0300, Dan Carpenter wrote:
> > > > The shmem_file_setup() function can't return NULL so there is no need
> > > > to check and doing so is a bit confusing.
> > > > 
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > > ---
> > > > No fixes tag because this is not a bug, just some confusing code.
> > > 
> > > Please don't re-send patches that have already been presented here.
> > >
> > 
> > FWIW, I side with Dan's argument.  shmem_file_setup() *does not*
> > return NULL.  If it ever *did* return NULL, it would be up to the
> > person who makes that happen to change all callers to check for NULL.
> And as I asked three weeks ago, what's the harm in checking for a NULL
> pointer here?  The kerneldoc for shmem_file_setup doesn't explicitly
> exclude a null return.  True, it doesn't mention the possibility of
> ERR_PTR returns either, but that's an accepted practice for pointer
> returns.

Mixing up error pointers and NULL checking is a common bug.  We never
hit this in run time these days because we've been using static analysis
to prevent it for over a decade.

If you look at the code we used to ship in the 2006 era it's absolutely
amazing (bad).  Static analysis isn't perfect for everything but there
are some kinds of simple bugs where we've almost eliminated them

> For a call outside of the xfs subsystem, I think it's prudent to have
> stronger return value checking.  Yes, someone changing the interface
> would have to add a null check to all the callsites, but (a) it's benign
> to guard against a behavior change in another module and (b) people miss
> things all the time.
> > The current code *suggests* that it could return NULL, which
> > is not correct.
> Huh?
> Are you talking about this stupid behavior of bots where they decide
> what a function returns based on the callsites in lieu of analyzing the
> actual implementation code?

In this case Smatch is looking at the implementation.

$ smdb return_states shmem_file_setup | grep INTER
mm/shmem.c | shmem_file_setup | 2229 |  4096-ptr_max|        INTERNAL | -1 |                      | struct file*(*)(char*, llong, ulong) |
mm/shmem.c | shmem_file_setup | 2230 |  (-4095)-(-1)|        INTERNAL | -1 |                      | struct file*(*)(char*, llong, ulong) |
mm/shmem.c | shmem_file_setup | 2231 | (-4095)-(-4),(-2)-(-1)|        INTERNAL | -1 |                      | struct file*(*)(char*, llong, ulong) |
mm/shmem.c | shmem_file_setup | 2232 |         (-22)|        INTERNAL | -1 |                      | struct file*(*)(char*, llong, ulong) |
mm/shmem.c | shmem_file_setup | 2233 |         (-12)|        INTERNAL | -1 |                      | struct file*(*)(char*, llong, ulong) |

I also manually reviewed the function implementation to verify.

> I don't feel like getting harassed by bots when someone /does/
> accidentally change the implementation to return NULL, and now one of
> the other build/test/syz bots starts crashing in xfile_create.

If someone changes it to return NULL then there are a few things to

1) That person is going to have change all the call sites.
2) Normally when a function returns both error pointers and NULL then
   the NULL is success. So this code doesn't handle the normal case
   correctly because it assumes NULL is an error.
3) Static checkers will complain if the call sites are not updated.  Try
   to find any IS_ERR vs NULL bugs in the past year that were found at
   run time instead of through static analysis.  Occasionally syzbot
   will hit them before me but it's rare.  I don't even remember the
   last time where this sort of bug managed to reach actual users.

> Of course all that has bought me is ... more f*** bot harassment.
> I'm BURNED OUT, give me a fucking break!

Fair enough.  I should have seen Yang Yingliang's email.

dan carpenter

[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]

  Powered by Linux