* [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd)
@ 2023-08-12 11:10 Julia Lawall
2023-08-12 15:59 ` Hugh Dickins
0 siblings, 1 reply; 4+ messages in thread
From: Julia Lawall @ 2023-08-12 11:10 UTC (permalink / raw)
To: Hugh Dickins
Cc: Christian Brauner, Jan Kara, Carlos Maiolino, Christian Brauner,
oe-kbuild-all
Maybe it would be nice to transform this to a more traditional
if (IS_ERR(...)) return ...
and then the branch code with a return 0 at the end rather than relying on
the PTR_ERR of NULL to return 0.
julia
---------- Forwarded message ----------
Date: Sat, 12 Aug 2023 18:47:30 +0800
From: kernel test robot <lkp@intel.com>
To: oe-kbuild@lists.linux.dev
Cc: lkp@intel.com, Julia Lawall <julia.lawall@inria.fr>
Subject: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR
applied after initialization to constant on line 3607
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
CC: Christian Brauner <christianvanbrauner@gmail.com>
TO: Hugh Dickins <hughd@google.com>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Carlos Maiolino <cmaiolino@redhat.com>
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.tmpfs
head: aa5b9178c01905d7691512b366cf2886dfe2680c
commit: 5de75970c9fd7220e394b76e6d20fbafa1369b5a [15/19] xattr: simple_xattr_set() return old_xattr to be freed
:::::: branch date: 23 hours ago
:::::: commit date: 3 days ago
config: openrisc-randconfig-r052-20230812 (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Julia Lawall <julia.lawall@inria.fr>
| Closes: https://lore.kernel.org/r/202308121841.cgYdEdbt-lkp@intel.com/
cocci warnings: (new ones prefixed by >>)
>> mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607
vim +3611 mm/shmem.c
b09e0fa4b4ea66 Eric Paris 2011-05-24 3593
aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3594 static int shmem_xattr_handler_set(const struct xattr_handler *handler,
39f60c1ccee72c Christian Brauner 2023-01-13 3595 struct mnt_idmap *idmap,
5930122683dff5 Al Viro 2016-05-27 3596 struct dentry *unused, struct inode *inode,
5930122683dff5 Al Viro 2016-05-27 3597 const char *name, const void *value,
5930122683dff5 Al Viro 2016-05-27 3598 size_t size, int flags)
b09e0fa4b4ea66 Eric Paris 2011-05-24 3599 {
5930122683dff5 Al Viro 2016-05-27 3600 struct shmem_inode_info *info = SHMEM_I(inode);
5de75970c9fd72 Hugh Dickins 2023-08-08 3601 struct simple_xattr *old_xattr;
b09e0fa4b4ea66 Eric Paris 2011-05-24 3602
aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3603 name = xattr_full_name(handler, name);
5de75970c9fd72 Hugh Dickins 2023-08-08 3604 old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
5de75970c9fd72 Hugh Dickins 2023-08-08 3605 if (!IS_ERR(old_xattr)) {
5de75970c9fd72 Hugh Dickins 2023-08-08 3606 simple_xattr_free(old_xattr);
5de75970c9fd72 Hugh Dickins 2023-08-08 @3607 old_xattr = NULL;
36f05cab0a2c97 Jeff Layton 2022-09-09 3608 inode->i_ctime = current_time(inode);
36f05cab0a2c97 Jeff Layton 2022-09-09 3609 inode_inc_iversion(inode);
36f05cab0a2c97 Jeff Layton 2022-09-09 3610 }
5de75970c9fd72 Hugh Dickins 2023-08-08 @3611 return PTR_ERR(old_xattr);
b09e0fa4b4ea66 Eric Paris 2011-05-24 3612 }
b09e0fa4b4ea66 Eric Paris 2011-05-24 3613
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd)
2023-08-12 11:10 [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd) Julia Lawall
@ 2023-08-12 15:59 ` Hugh Dickins
2023-08-12 16:04 ` Julia Lawall
2023-09-05 8:37 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Hugh Dickins @ 2023-08-12 15:59 UTC (permalink / raw)
To: Julia Lawall
Cc: Hugh Dickins, Christian Brauner, Jan Kara, Carlos Maiolino,
Dan Carpenter, Christian Brauner, oe-kbuild-all
On Sat, 12 Aug 2023, Julia Lawall wrote:
> Maybe it would be nice to transform this to a more traditional
> if (IS_ERR(...)) return ...
> and then the branch code with a return 0 at the end rather than relying on
> the PTR_ERR of NULL to return 0.
Thanks for checking, Julia. But as I said to Dan yesterday in
https://lore.kernel.org/oe-kbuild-all/baf5ac73-23b4-e325-28e5-9547af7ec3a@google.com/
I'm going to resist making any change here.
As you observe, the code is correct as is. And I did have a version
(unposted) which laid it out exactly as you suggest there. But the
collisions between different branches of vfs.git (and subsequent mods
here in vfs.tmpfs) was such that I aimed to minimize the interference.
And I'll expand: if you look a couple of patches later, you'll see why
the PTR_ERR(old_xattr) makes more sense: there is then cleanup to be
done before returning, shared between the failure and success paths.
I had in mind that people might be trying to apply this series to
different trees, and flipping the indentation back and forth from
patch to patch was not going to help them, when there's a known
conflict to come on the current_time(inode) line.
>
> julia
>
>
> ---------- Forwarded message ----------
> Date: Sat, 12 Aug 2023 18:47:30 +0800
> From: kernel test robot <lkp@intel.com>
> To: oe-kbuild@lists.linux.dev
> Cc: lkp@intel.com, Julia Lawall <julia.lawall@inria.fr>
> Subject: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR
> applied after initialization to constant on line 3607
>
> BCC: lkp@intel.com
> CC: oe-kbuild-all@lists.linux.dev
> CC: Christian Brauner <christianvanbrauner@gmail.com>
> TO: Hugh Dickins <hughd@google.com>
> CC: Christian Brauner <brauner@kernel.org>
> CC: Jan Kara <jack@suse.cz>
> CC: Carlos Maiolino <cmaiolino@redhat.com>
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.tmpfs
> head: aa5b9178c01905d7691512b366cf2886dfe2680c
> commit: 5de75970c9fd7220e394b76e6d20fbafa1369b5a [15/19] xattr: simple_xattr_set() return old_xattr to be freed
> :::::: branch date: 23 hours ago
> :::::: commit date: 3 days ago
> config: openrisc-randconfig-r052-20230812 (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Julia Lawall <julia.lawall@inria.fr>
> | Closes: https://lore.kernel.org/r/202308121841.cgYdEdbt-lkp@intel.com/
>
> cocci warnings: (new ones prefixed by >>)
> >> mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607
I'm guessing that there is some common pattern of mistakes which this
check has a good track record of catching, and then it is useful; but I
don't see it as useful here - there is nothing wrong with PTR_ERR(NULL).
Hugh
>
> vim +3611 mm/shmem.c
>
> b09e0fa4b4ea66 Eric Paris 2011-05-24 3593
> aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3594 static int shmem_xattr_handler_set(const struct xattr_handler *handler,
> 39f60c1ccee72c Christian Brauner 2023-01-13 3595 struct mnt_idmap *idmap,
> 5930122683dff5 Al Viro 2016-05-27 3596 struct dentry *unused, struct inode *inode,
> 5930122683dff5 Al Viro 2016-05-27 3597 const char *name, const void *value,
> 5930122683dff5 Al Viro 2016-05-27 3598 size_t size, int flags)
> b09e0fa4b4ea66 Eric Paris 2011-05-24 3599 {
> 5930122683dff5 Al Viro 2016-05-27 3600 struct shmem_inode_info *info = SHMEM_I(inode);
> 5de75970c9fd72 Hugh Dickins 2023-08-08 3601 struct simple_xattr *old_xattr;
> b09e0fa4b4ea66 Eric Paris 2011-05-24 3602
> aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3603 name = xattr_full_name(handler, name);
> 5de75970c9fd72 Hugh Dickins 2023-08-08 3604 old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
> 5de75970c9fd72 Hugh Dickins 2023-08-08 3605 if (!IS_ERR(old_xattr)) {
> 5de75970c9fd72 Hugh Dickins 2023-08-08 3606 simple_xattr_free(old_xattr);
> 5de75970c9fd72 Hugh Dickins 2023-08-08 @3607 old_xattr = NULL;
> 36f05cab0a2c97 Jeff Layton 2022-09-09 3608 inode->i_ctime = current_time(inode);
> 36f05cab0a2c97 Jeff Layton 2022-09-09 3609 inode_inc_iversion(inode);
> 36f05cab0a2c97 Jeff Layton 2022-09-09 3610 }
> 5de75970c9fd72 Hugh Dickins 2023-08-08 @3611 return PTR_ERR(old_xattr);
> b09e0fa4b4ea66 Eric Paris 2011-05-24 3612 }
> b09e0fa4b4ea66 Eric Paris 2011-05-24 3613
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd)
2023-08-12 15:59 ` Hugh Dickins
@ 2023-08-12 16:04 ` Julia Lawall
2023-09-05 8:37 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Julia Lawall @ 2023-08-12 16:04 UTC (permalink / raw)
To: Hugh Dickins
Cc: Christian Brauner, Jan Kara, Carlos Maiolino, Dan Carpenter,
Christian Brauner, oe-kbuild-all
On Sat, 12 Aug 2023, Hugh Dickins wrote:
> On Sat, 12 Aug 2023, Julia Lawall wrote:
>
> > Maybe it would be nice to transform this to a more traditional
> > if (IS_ERR(...)) return ...
> > and then the branch code with a return 0 at the end rather than relying on
> > the PTR_ERR of NULL to return 0.
>
> Thanks for checking, Julia. But as I said to Dan yesterday in
> https://lore.kernel.org/oe-kbuild-all/baf5ac73-23b4-e325-28e5-9547af7ec3a@google.com/
>
> I'm going to resist making any change here.
>
> As you observe, the code is correct as is. And I did have a version
> (unposted) which laid it out exactly as you suggest there. But the
> collisions between different branches of vfs.git (and subsequent mods
> here in vfs.tmpfs) was such that I aimed to minimize the interference.
>
> And I'll expand: if you look a couple of patches later, you'll see why
> the PTR_ERR(old_xattr) makes more sense: there is then cleanup to be
> done before returning, shared between the failure and success paths.
>
> I had in mind that people might be trying to apply this series to
> different trees, and flipping the indentation back and forth from
> patch to patch was not going to help them, when there's a known
> conflict to come on the current_time(inode) line.
OK, no problem. Sorry for not having checked on the recent discussion.
julia
>
> >
> > julia
> >
> >
> > ---------- Forwarded message ----------
> > Date: Sat, 12 Aug 2023 18:47:30 +0800
> > From: kernel test robot <lkp@intel.com>
> > To: oe-kbuild@lists.linux.dev
> > Cc: lkp@intel.com, Julia Lawall <julia.lawall@inria.fr>
> > Subject: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR
> > applied after initialization to constant on line 3607
> >
> > BCC: lkp@intel.com
> > CC: oe-kbuild-all@lists.linux.dev
> > CC: Christian Brauner <christianvanbrauner@gmail.com>
> > TO: Hugh Dickins <hughd@google.com>
> > CC: Christian Brauner <brauner@kernel.org>
> > CC: Jan Kara <jack@suse.cz>
> > CC: Carlos Maiolino <cmaiolino@redhat.com>
> >
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.tmpfs
> > head: aa5b9178c01905d7691512b366cf2886dfe2680c
> > commit: 5de75970c9fd7220e394b76e6d20fbafa1369b5a [15/19] xattr: simple_xattr_set() return old_xattr to be freed
> > :::::: branch date: 23 hours ago
> > :::::: commit date: 3 days ago
> > config: openrisc-randconfig-r052-20230812 (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/config)
> > compiler: or1k-linux-gcc (GCC) 12.3.0
> > reproduce: (https://download.01.org/0day-ci/archive/20230812/202308121841.cgYdEdbt-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Julia Lawall <julia.lawall@inria.fr>
> > | Closes: https://lore.kernel.org/r/202308121841.cgYdEdbt-lkp@intel.com/
> >
> > cocci warnings: (new ones prefixed by >>)
> > >> mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607
>
> I'm guessing that there is some common pattern of mistakes which this
> check has a good track record of catching, and then it is useful; but I
> don't see it as useful here - there is nothing wrong with PTR_ERR(NULL).
>
> Hugh
>
> >
> > vim +3611 mm/shmem.c
> >
> > b09e0fa4b4ea66 Eric Paris 2011-05-24 3593
> > aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3594 static int shmem_xattr_handler_set(const struct xattr_handler *handler,
> > 39f60c1ccee72c Christian Brauner 2023-01-13 3595 struct mnt_idmap *idmap,
> > 5930122683dff5 Al Viro 2016-05-27 3596 struct dentry *unused, struct inode *inode,
> > 5930122683dff5 Al Viro 2016-05-27 3597 const char *name, const void *value,
> > 5930122683dff5 Al Viro 2016-05-27 3598 size_t size, int flags)
> > b09e0fa4b4ea66 Eric Paris 2011-05-24 3599 {
> > 5930122683dff5 Al Viro 2016-05-27 3600 struct shmem_inode_info *info = SHMEM_I(inode);
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 3601 struct simple_xattr *old_xattr;
> > b09e0fa4b4ea66 Eric Paris 2011-05-24 3602
> > aa7c5241c380ad Andreas Gruenbacher 2015-12-02 3603 name = xattr_full_name(handler, name);
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 3604 old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 3605 if (!IS_ERR(old_xattr)) {
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 3606 simple_xattr_free(old_xattr);
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 @3607 old_xattr = NULL;
> > 36f05cab0a2c97 Jeff Layton 2022-09-09 3608 inode->i_ctime = current_time(inode);
> > 36f05cab0a2c97 Jeff Layton 2022-09-09 3609 inode_inc_iversion(inode);
> > 36f05cab0a2c97 Jeff Layton 2022-09-09 3610 }
> > 5de75970c9fd72 Hugh Dickins 2023-08-08 @3611 return PTR_ERR(old_xattr);
> > b09e0fa4b4ea66 Eric Paris 2011-05-24 3612 }
> > b09e0fa4b4ea66 Eric Paris 2011-05-24 3613
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd)
2023-08-12 15:59 ` Hugh Dickins
2023-08-12 16:04 ` Julia Lawall
@ 2023-09-05 8:37 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-09-05 8:37 UTC (permalink / raw)
To: Hugh Dickins
Cc: Julia Lawall, Christian Brauner, Jan Kara, Carlos Maiolino,
Christian Brauner, oe-kbuild-all
On Sat, Aug 12, 2023 at 08:59:58AM -0700, Hugh Dickins wrote:
> > cocci warnings: (new ones prefixed by >>)
> > >> mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607
>
> I'm guessing that there is some common pattern of mistakes which this
> check has a good track record of catching, and then it is useful; but I
> don't see it as useful here - there is nothing wrong with PTR_ERR(NULL).
Yeah. PTR_ERR(NULL) is valid. That's used quite a bit in fs/. There
was (is?) some functions that only returns error pointers and NULL and
never a valid pointer.
But in new code 90% (this is not hyperbole) of the time then the
PTR_ERR(NULL) warning finds buggy code. Quite often the bug is harmless
like people think checking kmalloc() for both NULL and error pointers is
useful. But it's definitely a useful heuristic.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-05 8:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-12 11:10 [brauner-vfs:vfs.tmpfs 15/19] mm/shmem.c:3611:8-15: ERROR: PTR_ERR applied after initialization to constant on line 3607 (fwd) Julia Lawall
2023-08-12 15:59 ` Hugh Dickins
2023-08-12 16:04 ` Julia Lawall
2023-09-05 8:37 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).