| Message ID | 20250128235408.11229-1-qasdev00@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/shmem: Fix invalid PTR_ERR(NULL) call in shmem_xattr_handler_set() | expand |
On Tue, 28 Jan 2025, Qasim Ijaz wrote: > In shmem_xattr_handler_set() if simple_xattr_set() succeeds and the pointer > returned is not an error pointer, then old_xattr will be set to NULL in > the body of the following if statement: > > if (!IS_ERR(old_xattr)) > > Later on shmem_xattr_handler_set() calls: > > return PTR_ERR(old_xattr); > > The PTR_ERR macro is used to extract an error code from an error pointer > and NULL is not an error pointer, PTR_ERR(NULL) simply results in 0. NULL pointer returns 0 for success: yes, that's what's wanted there. > > To improve correctness and readability, Correctness? Please explain - I don't see any incorrectness. Readability? Perhaps - I should not be the judge of that. > refactor the error handling > to have an explicit default return value of 0 (success) in "ret". > If simple_xattr_set() returns an error pointer, store its > error code in "ret". > > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> > --- > mm/shmem.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) I prefer how it was written before. > > diff --git a/mm/shmem.c b/mm/shmem.c > index 532afd8e049c..3e97c7890aac 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -4143,6 +4143,7 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, > struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb); > struct simple_xattr *old_xattr; > size_t ispace = 0; > + int ret = 0; > > name = xattr_full_name(handler, name); > if (value && sbinfo->max_inodes) { > @@ -4158,7 +4156,9 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, > } > > old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags); > - if (!IS_ERR(old_xattr)) { > + if (IS_ERR(old_xattr)) { > + ret = PTR_ERR(old_xattr); > + } else { > ispace = 0; > if (old_xattr && sbinfo->max_inodes) > ispace = simple_xattr_space(old_xattr->name, > @@ -4168,12 +4171,13 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, > inode_set_ctime_current(inode); > inode_inc_iversion(inode); > } > + > if (ispace) { > raw_spin_lock(&sbinfo->stat_lock); > sbinfo->free_ispace += ispace; > raw_spin_unlock(&sbinfo->stat_lock); > } > - return PTR_ERR(old_xattr); > + return ret; > } > > static const struct xattr_handler shmem_security_xattr_handler = { > -- > 2.39.5
diff --git a/mm/shmem.c b/mm/shmem.c index 532afd8e049c..3e97c7890aac 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -4143,6 +4143,7 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb); struct simple_xattr *old_xattr; size_t ispace = 0; + int ret = 0; name = xattr_full_name(handler, name); if (value && sbinfo->max_inodes) { @@ -4158,7 +4156,9 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, } old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags); - if (!IS_ERR(old_xattr)) { + if (IS_ERR(old_xattr)) { + ret = PTR_ERR(old_xattr); + } else { ispace = 0; if (old_xattr && sbinfo->max_inodes) ispace = simple_xattr_space(old_xattr->name, @@ -4168,12 +4171,13 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler, inode_set_ctime_current(inode); inode_inc_iversion(inode); } + if (ispace) { raw_spin_lock(&sbinfo->stat_lock); sbinfo->free_ispace += ispace; raw_spin_unlock(&sbinfo->stat_lock); } - return PTR_ERR(old_xattr); + return ret; } static const struct xattr_handler shmem_security_xattr_handler = {
In shmem_xattr_handler_set() if simple_xattr_set() succeeds and the pointer returned is not an error pointer, then old_xattr will be set to NULL in the body of the following if statement: if (!IS_ERR(old_xattr)) Later on shmem_xattr_handler_set() calls: return PTR_ERR(old_xattr); The PTR_ERR macro is used to extract an error code from an error pointer and NULL is not an error pointer, PTR_ERR(NULL) simply results in 0. To improve correctness and readability, refactor the error handling to have an explicit default return value of 0 (success) in "ret". If simple_xattr_set() returns an error pointer, store its error code in "ret". Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> --- mm/shmem.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)