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. > > > https://lore.kernel.org/linux-xfs/20230824161428.GO11263@frogsfrogsfrogs/ > > > > 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 completely. > > 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 note. 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. regards, dan carpenter
