diff mbox series

[1/2] mm/mmap: remove unnecessary vp->vma check in vma_prepare

Message ID 20230301022720.1380780-1-surenb@google.com
State New
Headers
Series [1/2] mm/mmap: remove unnecessary vp->vma check in vma_prepare |

Commit Message

Suren Baghdasaryan March 1, 2023, 2:27 a.m. UTC 
  vp->vma in vma_prepare() is always non-NULL, therefore checking it is
not necessary. Remove the extra check.

Fixes: e8f071350ea5 ("mm/mmap: write-lock VMAs in vma_prepare before modifying them")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/r/202302281802.J93Nma7q-lkp@intel.com/
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
Fix cleanly apply over mm-unstable, SHA in "Fixes" is from that tree.

 mm/mmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

David Hildenbrand March 1, 2023, 8:26 a.m. UTC | #1 
On 01.03.23 03:27, Suren Baghdasaryan wrote:
> vp->vma in vma_prepare() is always non-NULL, therefore checking it is
> not necessary. Remove the extra check.
> 
> Fixes: e8f071350ea5 ("mm/mmap: write-lock VMAs in vma_prepare before modifying them")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>

It would be great to mention that this simply silences a smatch warning. 
Otherwise, one might be mislead that this commit fixes an actual BUG ;)

> Link: https://lore.kernel.org/r/202302281802.J93Nma7q-lkp@intel.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> Fix cleanly apply over mm-unstable, SHA in "Fixes" is from that tree.
> 
>   mm/mmap.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 0cd3714c2182..0759d53b470c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -505,8 +505,7 @@ static inline void init_vma_prep(struct vma_prepare *vp,
>    */
>   static inline void vma_prepare(struct vma_prepare *vp)
>   {
> -	if (vp->vma)
> -		vma_start_write(vp->vma);
> +	vma_start_write(vp->vma);
>   	if (vp->adj_next)
>   		vma_start_write(vp->adj_next);
>   	/* vp->insert is always a newly created VMA, no need for locking */

Yes, that looks correct.

Reviewed-by: David Hildenbrand <david@redhat.com>
Liam R. Howlett March 1, 2023, 2:10 p.m. UTC | #2 
* Suren Baghdasaryan <surenb@google.com> [230228 21:27]:
> vp->vma in vma_prepare() is always non-NULL, therefore checking it is
> not necessary. Remove the extra check.
> 
> Fixes: e8f071350ea5 ("mm/mmap: write-lock VMAs in vma_prepare before modifying them")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Link: https://lore.kernel.org/r/202302281802.J93Nma7q-lkp@intel.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> Fix cleanly apply over mm-unstable, SHA in "Fixes" is from that tree.
> 
>  mm/mmap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 0cd3714c2182..0759d53b470c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -505,8 +505,7 @@ static inline void init_vma_prep(struct vma_prepare *vp,
>   */
>  static inline void vma_prepare(struct vma_prepare *vp)
>  {
> -	if (vp->vma)
> -		vma_start_write(vp->vma);
> +	vma_start_write(vp->vma);

Would a WARN_ON_ONCE() be worth it?  Maybe not since it will be detected
rather quickly once we dereference it below, but it might make it more
clear as to what happened?

I'm happy either way.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

>  	if (vp->adj_next)
>  		vma_start_write(vp->adj_next);
>  	/* vp->insert is always a newly created VMA, no need for locking */
> -- 
> 2.39.2.722.g9855ee24e9-goog
>
Suren Baghdasaryan March 1, 2023, 5:54 p.m. UTC | #3 
On Wed, Mar 1, 2023 at 6:10 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Suren Baghdasaryan <surenb@google.com> [230228 21:27]:
> > vp->vma in vma_prepare() is always non-NULL, therefore checking it is
> > not necessary. Remove the extra check.
> >
> > Fixes: e8f071350ea5 ("mm/mmap: write-lock VMAs in vma_prepare before modifying them")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Link: https://lore.kernel.org/r/202302281802.J93Nma7q-lkp@intel.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > Fix cleanly apply over mm-unstable, SHA in "Fixes" is from that tree.
> >
> >  mm/mmap.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 0cd3714c2182..0759d53b470c 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -505,8 +505,7 @@ static inline void init_vma_prep(struct vma_prepare *vp,
> >   */
> >  static inline void vma_prepare(struct vma_prepare *vp)
> >  {
> > -     if (vp->vma)
> > -             vma_start_write(vp->vma);
> > +     vma_start_write(vp->vma);
>
> Would a WARN_ON_ONCE() be worth it?  Maybe not since it will be detected
> rather quickly once we dereference it below, but it might make it more
> clear as to what happened?

WARN_ON_ONCE() seems like an overkill to me. It always follows after
init_multi_vma_prep()/init_vma_prep() both of which set the VMA. Risk
should be minimal here and as you said, misuse is easily discoverable.

>
> I'm happy either way.
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
>
> >       if (vp->adj_next)
> >               vma_start_write(vp->adj_next);
> >       /* vp->insert is always a newly created VMA, no need for locking */
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 0cd3714c2182..0759d53b470c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -505,8 +505,7 @@  static inline void init_vma_prep(struct vma_prepare *vp,
  */
 static inline void vma_prepare(struct vma_prepare *vp)
 {
-	if (vp->vma)
-		vma_start_write(vp->vma);
+	vma_start_write(vp->vma);
 	if (vp->adj_next)
 		vma_start_write(vp->adj_next);
 	/* vp->insert is always a newly created VMA, no need for locking */