| Message ID | 6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mempolicy: Fix exception handling in shared_policy_replace() | expand |
Your patch doesn't apply, seems like it uses spaces instead of tabs. Also I can't use 'b4' to download it as there are multiple different patches using the same message-id: https://lore.kernel.org/all/6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de/ Re: subject, I don't see a bug that this would fix. You could say it's "cleanup" and this function could use one, but for a cleanup it's not improving the situation much. On 3/23/23 18:30, Markus Elfring wrote: > Date: Thu, 23 Mar 2023 18:18:59 +0100 > > The label “err_out” was used to jump to another pointer check despite of > the detail in the implementation of the function “shared_policy_replace” > that it was determined already that a corresponding variable contained a > null pointer because of a failed call of the function “kmem_cache_alloc”. > > 1. Use more appropriate labels instead. > > 2. The implementation of the function “mpol_put” contains a pointer check > for its single input parameter. > Thus delete a redundant check in the caller. > > > This issue was detected by using the Coccinelle software. > > Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock") Again this is not a fix. > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > mm/mempolicy.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index a256a241fd1d..fb0485688dcb 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -2736,13 +2736,12 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, > sp_insert(sp, new); > write_unlock(&sp->lock); > ret = 0; > +put_mpol: > + mpol_put(mpol_new); > > -err_out: > - if (mpol_new) > - mpol_put(mpol_new); > if (n_new) > kmem_cache_free(sn_cache, n_new); > - > +exit: > return ret; > > alloc_new: > @@ -2750,10 +2749,10 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, > ret = -ENOMEM; > n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL); > if (!n_new) > - goto err_out; > + goto exit; Just "return ret" and no need for exit label? > mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL); > if (!mpol_new) > - goto err_out; > + goto put_mpol; We are doing this because mpol_new == NULL, so we know there's no reason to do mpol_put(), we could jump to the freeing of n_new. > atomic_set(&mpol_new->refcnt, 1); > goto restart; > } > -- > 2.40.0 > > >
> Your patch doesn't apply, seems like it uses spaces instead of tabs. I am sorry for this glitch. Andrew Morton picked my change suggestion up yesterday. https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mempolicy-fix-exception-handling-in-shared_policy_replace.patch > https://lore.kernel.org/all/6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de/ > > Re: subject, I don't see a bug that this would fix. You could say it's > "cleanup" and this function could use one, but for a cleanup it's not > improving the situation much. I find a few details improvable also for the mentioned function implementation. >> The label “err_out” was used to jump to another pointer check despite of >> the detail in the implementation of the function “shared_policy_replace” >> that it was determined already that a corresponding variable contained a >> null pointer because of a failed call of the function “kmem_cache_alloc”. >> >> 1. Use more appropriate labels instead. >> >> 2. The implementation of the function “mpol_put” contains a pointer check >> for its single input parameter. >> Thus delete a redundant check in the caller. >> >> >> This issue was detected by using the Coccinelle software. >> >> Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock") > > Again this is not a fix. Do you find the change description helpful? Regards, Markus
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index a256a241fd1d..fb0485688dcb 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -2736,13 +2736,12 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, sp_insert(sp, new); write_unlock(&sp->lock); ret = 0; +put_mpol: + mpol_put(mpol_new); -err_out: - if (mpol_new) - mpol_put(mpol_new); if (n_new) kmem_cache_free(sn_cache, n_new); - +exit: return ret; alloc_new: @@ -2750,10 +2749,10 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start, ret = -ENOMEM; n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL); if (!n_new) - goto err_out; + goto exit; mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL); if (!mpol_new) - goto err_out; + goto put_mpol; atomic_set(&mpol_new->refcnt, 1); goto restart; }
Date: Thu, 23 Mar 2023 18:18:59 +0100 The label “err_out” was used to jump to another pointer check despite of the detail in the implementation of the function “shared_policy_replace” that it was determined already that a corresponding variable contained a null pointer because of a failed call of the function “kmem_cache_alloc”. 1. Use more appropriate labels instead. 2. The implementation of the function “mpol_put” contains a pointer check for its single input parameter. Thus delete a redundant check in the caller. This issue was detected by using the Coccinelle software. Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock") Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- mm/mempolicy.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) -- 2.40.0