* [PATCH] xfs: delete some dead code in xfile_create() @ 2023-09-12 15:18 Dan Carpenter 2023-09-12 15:38 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2023-09-12 15:18 UTC (permalink / raw) To: Darrick J. Wong Cc: Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors 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@linaro.org> --- No fixes tag because this is not a bug, just some confusing code. fs/xfs/scrub/xfile.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c index d98e8e77c684..71779d81cad7 100644 --- a/fs/xfs/scrub/xfile.c +++ b/fs/xfs/scrub/xfile.c @@ -70,8 +70,6 @@ xfile_create( return -ENOMEM; xf->file = shmem_file_setup(description, isize, 0); - if (!xf->file) - goto out_xfile; if (IS_ERR(xf->file)) { error = PTR_ERR(xf->file); goto out_xfile; -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: delete some dead code in xfile_create() 2023-09-12 15:18 [PATCH] xfs: delete some dead code in xfile_create() Dan Carpenter @ 2023-09-12 15:38 ` Darrick J. Wong 2023-09-12 15:41 ` Alex Elder 2023-09-13 6:32 ` Dan Carpenter 0 siblings, 2 replies; 6+ messages in thread From: Darrick J. Wong @ 2023-09-12 15:38 UTC (permalink / raw) To: Dan Carpenter Cc: Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors 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@linaro.org> > --- > 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/ --D > fs/xfs/scrub/xfile.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > index d98e8e77c684..71779d81cad7 100644 > --- a/fs/xfs/scrub/xfile.c > +++ b/fs/xfs/scrub/xfile.c > @@ -70,8 +70,6 @@ xfile_create( > return -ENOMEM; > > xf->file = shmem_file_setup(description, isize, 0); > - if (!xf->file) > - goto out_xfile; > if (IS_ERR(xf->file)) { > error = PTR_ERR(xf->file); > goto out_xfile; > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: delete some dead code in xfile_create() 2023-09-12 15:38 ` Darrick J. Wong @ 2023-09-12 15:41 ` Alex Elder 2023-09-12 16:23 ` Darrick J. Wong 2023-09-13 6:32 ` Dan Carpenter 1 sibling, 1 reply; 6+ messages in thread From: Alex Elder @ 2023-09-12 15:41 UTC (permalink / raw) To: Darrick J. Wong, Dan Carpenter Cc: Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors 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@linaro.org> >> --- >> 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. The current code *suggests* that it could return NULL, which is not correct. -Alex > > --D > >> fs/xfs/scrub/xfile.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c >> index d98e8e77c684..71779d81cad7 100644 >> --- a/fs/xfs/scrub/xfile.c >> +++ b/fs/xfs/scrub/xfile.c >> @@ -70,8 +70,6 @@ xfile_create( >> return -ENOMEM; >> >> xf->file = shmem_file_setup(description, isize, 0); >> - if (!xf->file) >> - goto out_xfile; >> if (IS_ERR(xf->file)) { >> error = PTR_ERR(xf->file); >> goto out_xfile; >> -- >> 2.39.2 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: delete some dead code in xfile_create() 2023-09-12 15:41 ` Alex Elder @ 2023-09-12 16:23 ` Darrick J. Wong 2023-09-13 7:01 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2023-09-12 16:23 UTC (permalink / raw) To: Alex Elder Cc: Dan Carpenter, Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors 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@linaro.org> > > > --- > > > 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. 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? 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. Of course all that has bought me is ... more f*** bot harassment. I'm BURNED OUT, give me a fucking break! --D > -Alex > > > > > --D > > > > > fs/xfs/scrub/xfile.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > > > index d98e8e77c684..71779d81cad7 100644 > > > --- a/fs/xfs/scrub/xfile.c > > > +++ b/fs/xfs/scrub/xfile.c > > > @@ -70,8 +70,6 @@ xfile_create( > > > return -ENOMEM; > > > xf->file = shmem_file_setup(description, isize, 0); > > > - if (!xf->file) > > > - goto out_xfile; > > > if (IS_ERR(xf->file)) { > > > error = PTR_ERR(xf->file); > > > goto out_xfile; > > > -- > > > 2.39.2 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: delete some dead code in xfile_create() 2023-09-12 16:23 ` Darrick J. Wong @ 2023-09-13 7:01 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2023-09-13 7:01 UTC (permalink / raw) To: Darrick J. Wong Cc: Alex Elder, Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors 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@linaro.org> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: delete some dead code in xfile_create() 2023-09-12 15:38 ` Darrick J. Wong 2023-09-12 15:41 ` Alex Elder @ 2023-09-13 6:32 ` Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2023-09-13 6:32 UTC (permalink / raw) To: Darrick J. Wong Cc: Chandan Babu R, Dave Chinner, Kent Overstreet, linux-xfs, kernel-janitors On Tue, Sep 12, 2023 at 08:38:24AM -0700, 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@linaro.org> > > --- > > 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/ > Should we set an error code? These kinds of impossible error situations are hard to handle correctly. Like there are some places were we work around bugs in driver code where we can trust them to return error pointers and that's totally a valid thing. But here it's a very puzzling thing. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-13 7:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-12 15:18 [PATCH] xfs: delete some dead code in xfile_create() Dan Carpenter 2023-09-12 15:38 ` Darrick J. Wong 2023-09-12 15:41 ` Alex Elder 2023-09-12 16:23 ` Darrick J. Wong 2023-09-13 7:01 ` Dan Carpenter 2023-09-13 6:32 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox