In theory when info->flags & VM_LOCKED we should not be getting
shem_writepage() called so we should be verifying this with a
WARN_ON_ONCE(). Since we should not be swapping then best to ensure
we also don't do the folio split earlier too. So just move the check
early to avoid folio splits in case its a dubious call.

We also have a similar early bail when !total_swap_pages so just move
that earlier to avoid the possible folio split in the same situation.

Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 mm/shmem.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 68e9970baf1e..dfd995da77b4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	if (WARN_ON_ONCE(!wbc->for_reclaim))
 		goto redirty;
 
+	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
+		goto redirty;
+
+	if (!total_swap_pages)
+		goto redirty;
+
 	/*
 	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
 	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
@@ -1365,10 +1371,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	}
 
 	index = folio->index;
-	if (info->flags & VM_LOCKED)
-		goto redirty;
-	if (!total_swap_pages)
-		goto redirty;
 
 	/*
 	 * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC
-- 
2.39.1

On Thu, 9 Mar 2023, Luis Chamberlain wrote:

> In theory when info->flags & VM_LOCKED we should not be getting
> shem_writepage() called so we should be verifying this with a
> WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> we also don't do the folio split earlier too. So just move the check
> early to avoid folio splits in case its a dubious call.
> 
> We also have a similar early bail when !total_swap_pages so just move
> that earlier to avoid the possible folio split in the same situation.
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  mm/shmem.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 68e9970baf1e..dfd995da77b4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	if (WARN_ON_ONCE(!wbc->for_reclaim))
>  		goto redirty;
>  
> +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> +		goto redirty;

Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
(hope it's not watching) will try flipping SHM_LOCK on while swapping out
pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
don't think any serialization prevents that.

Hugh

> +
> +	if (!total_swap_pages)
> +		goto redirty;
> +
>  	/*
>  	 * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
>  	 * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
> @@ -1365,10 +1371,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	}
>  
>  	index = folio->index;
> -	if (info->flags & VM_LOCKED)
> -		goto redirty;
> -	if (!total_swap_pages)
> -		goto redirty;
>  
>  	/*
>  	 * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC
> -- 
> 2.39.1

On Mon, Apr 17, 2023 at 09:41:41PM -0700, Hugh Dickins wrote:
> On Thu, 9 Mar 2023, Luis Chamberlain wrote:
> 
> > In theory when info->flags & VM_LOCKED we should not be getting
> > shem_writepage() called so we should be verifying this with a
> > WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> > we also don't do the folio split earlier too. So just move the check
> > early to avoid folio splits in case its a dubious call.
> > 
> > We also have a similar early bail when !total_swap_pages so just move
> > that earlier to avoid the possible folio split in the same situation.
> > 
> > Acked-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Christian Brauner <brauner@kernel.org>
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >  mm/shmem.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 68e9970baf1e..dfd995da77b4 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> >  	if (WARN_ON_ONCE(!wbc->for_reclaim))
> >  		goto redirty;
> >  
> > +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> > +		goto redirty;
> 
> Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
> (hope it's not watching) will try flipping SHM_LOCK on while swapping out
> pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
> don't think any serialization prevents that.

I though that may be the case. Would such serialization be welcomed?

  Luis

On Tue, 18 Apr 2023, Luis Chamberlain wrote:
> On Mon, Apr 17, 2023 at 09:41:41PM -0700, Hugh Dickins wrote:
> > On Thu, 9 Mar 2023, Luis Chamberlain wrote:
> > 
> > > +	if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> > > +		goto redirty;
> > 
> > Well, okay, I don't mind that.  But shall we take bets on how soon syzbot
> > (hope it's not watching) will try flipping SHM_LOCK on while swapping out
> > pages from a SHM segment, and hit that warning?  Perhaps I'm wrong, but I
> > don't think any serialization prevents that.
> 
> I though that may be the case. Would such serialization be welcomed?

Absolutely not!  We don't insert slowdowns just to avoid warnings,
unless the warning is of something that really matters.  This one
does not matter, the situation is correctly handled, so the warning
would be better reverted.  Though I personally don't mind you leaving
it in until the first report of it arrives.

Hugh

On Thu, Mar 9, 2023 at 3:05 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> In theory when info->flags & VM_LOCKED we should not be getting
> shem_writepage() called so we should be verifying this with a
> WARN_ON_ONCE(). Since we should not be swapping then best to ensure
> we also don't do the folio split earlier too. So just move the check
> early to avoid folio splits in case its a dubious call.
>
> We also have a similar early bail when !total_swap_pages so just move
> that earlier to avoid the possible folio split in the same situation.
>
> Acked-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  mm/shmem.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 68e9970baf1e..dfd995da77b4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1350,6 +1350,12 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>         if (WARN_ON_ONCE(!wbc->for_reclaim))
>                 goto redirty;
>
> +       if (WARN_ON_ONCE(info->flags & VM_LOCKED))
> +               goto redirty;
> +
> +       if (!total_swap_pages)
> +               goto redirty;
> +
>         /*
>          * If /sys/kernel/mm/transparent_hugepage/shmem_enabled is "always" or
>          * "force", drivers/gpu/drm/i915/gem/i915_gem_shmem.c gets huge pages,
> @@ -1365,10 +1371,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>         }
>
>         index = folio->index;
> -       if (info->flags & VM_LOCKED)
> -               goto redirty;
> -       if (!total_swap_pages)
> -               goto redirty;
>
>         /*
>          * This is somewhat ridiculous, but without plumbing a SWAP_MAP_FALLOC
> --
> 2.39.1
>