MIME-Version: 1.0
References: <20230228223838.3794807-1-dhowells@redhat.com> <20230228223838.3794807-2-dhowells@redhat.com>
In-Reply-To: <20230228223838.3794807-2-dhowells@redhat.com>
From:   Steve French <smfrench@gmail.com>
Date:   Wed, 1 Mar 2023 08:54:12 -0600
Message-ID: <CAH2r5muh5g=QOuqFRvNMLdiiHiJF-9AX0rAE_9M8zxvfW8i3bA@mail.gmail.com>
Subject: Re: [PATCH 1/1] cifs: Fix memory leak in direct I/O
To:     David Howells <dhowells@redhat.com>
Cc:     Shyam Prasad N <nspmangalore@gmail.com>,
        Rohith Surabattula <rohiths.msft@gmail.com>,
        Tom Talpey <tom@talpey.com>,
        Stefan Metzmacher <metze@samba.org>,
        Jeff Layton <jlayton@kernel.org>, linux-cifs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Murphy Zhou <jencce.kernel@gmail.com>,
        Steve French <sfrench@samba.org>, Paulo Alcantara <pc@cjr.nz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

merged into cifs-2.6.git for-next pending more testing

On Tue, Feb 28, 2023 at 4:38=E2=80=AFPM David Howells <dhowells@redhat.com>=
 wrote:
>
> When __cifs_readv() and __cifs_writev() extract pages from a user-backed
> iterator into a BVEC-type iterator, they set ->bv_need_unpin to note
> whether they need to unpin the pages later.  However, in both cases they
> examine the BVEC-type iterator and not the source iterator - and so
> bv_need_unpin doesn't get set and the pages are leaked.
>
> I think this may be responsible for the generic/208 xfstest failing
> occasionally with:
>
>         WARNING: CPU: 0 PID: 3064 at mm/gup.c:218 try_grab_page+0x65/0x10=
0
>         RIP: 0010:try_grab_page+0x65/0x100
>         follow_page_pte+0x1a7/0x570
>         __get_user_pages+0x1a2/0x650
>         __gup_longterm_locked+0xdc/0xb50
>         internal_get_user_pages_fast+0x17f/0x310
>         pin_user_pages_fast+0x46/0x60
>         iov_iter_extract_pages+0xc9/0x510
>         ? __kmalloc_large_node+0xb1/0x120
>         ? __kmalloc_node+0xbe/0x130
>         netfs_extract_user_iter+0xbf/0x200 [netfs]
>         __cifs_writev+0x150/0x330 [cifs]
>         vfs_write+0x2a8/0x3c0
>         ksys_pwrite64+0x65/0xa0
>
> with the page refcount going negative.  This is less unlikely than it see=
ms
> because the page is being pinned, not simply got, and so the refcount
> increased by 1024 each time, and so only needs to be called around ~20971=
52
> for the refcount to go negative.
>
> Further, the test program (aio-dio-invalidate-failure) uses a 32MiB stati=
c
> buffer and all the PTEs covering it refer to the same page because it's
> never written to.
>
> The warning in try_grab_page():
>
>         if (WARN_ON_ONCE(folio_ref_count(folio) <=3D 0))
>                 return -ENOMEM;
>
> then trips and prevents us ever using the page again for DIO at least.
>
> Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rathe=
r than a page list")
> Reported-by: Murphy Zhou <jencce.kernel@gmail.com>
> Link: https://lore.kernel.org/r/CAH2r5mvaTsJ---n=3D265a4zqRA7pP+o4MJ36WCQ=
US6oPrOij8cw@mail.gmail.com
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <sfrench@samba.org>
> cc: Shyam Prasad N <nspmangalore@gmail.com>
> cc: Rohith Surabattula <rohiths.msft@gmail.com>
> cc: Paulo Alcantara <pc@cjr.nz>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: linux-cifs@vger.kernel.org
> ---
>  fs/cifs/file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index ec0694a65c7b..4d4a2d82636d 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -3612,7 +3612,7 @@ static ssize_t __cifs_writev(
>
>                 ctx->nr_pinned_pages =3D rc;
>                 ctx->bv =3D (void *)ctx->iter.bvec;
> -               ctx->bv_need_unpin =3D iov_iter_extract_will_pin(&ctx->it=
er);
> +               ctx->bv_need_unpin =3D iov_iter_extract_will_pin(from);
>         } else if ((iov_iter_is_bvec(from) || iov_iter_is_kvec(from)) &&
>                    !is_sync_kiocb(iocb)) {
>                 /*
> @@ -4148,7 +4148,7 @@ static ssize_t __cifs_readv(
>
>                 ctx->nr_pinned_pages =3D rc;
>                 ctx->bv =3D (void *)ctx->iter.bvec;
> -               ctx->bv_need_unpin =3D iov_iter_extract_will_pin(&ctx->it=
er);
> +               ctx->bv_need_unpin =3D iov_iter_extract_will_pin(to);
>                 ctx->should_dirty =3D true;
>         } else if ((iov_iter_is_bvec(to) || iov_iter_is_kvec(to)) &&
>                    !is_sync_kiocb(iocb)) {
>


--=20
Thanks,

Steve