linux-kernel - Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30a4bd0e19626f5fb30f19f0ae70fba2debb361a.camel@kernel.org>
Date:   Fri, 01 Jul 2022 06:38:57 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     xiubli@...hat.com, idryomov@...il.com, dhowells@...hat.com
Cc:     vshankar@...hat.com, linux-kernel@...r.kernel.org,
        ceph-devel@...r.kernel.org, willy@...radead.org,
        keescook@...omium.org, linux-fsdevel@...r.kernel.org,
        linux-cachefs@...hat.com
Subject: Re: [PATCH 1/2] netfs: release the folio lock and put the folio
 before retrying

On Fri, 2022-07-01 at 10:29 +0800, xiubli@...hat.com wrote:
> From: Xiubo Li <xiubli@...hat.com>
> 
> The lower layer filesystem should always make sure the folio is
> locked and do the unlock and put the folio in netfs layer.
> 
> URL: https://tracker.ceph.com/issues/56423
> Signed-off-by: Xiubo Li <xiubli@...hat.com>
> ---
>  fs/netfs/buffered_read.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 42f892c5712e..257fd37c2461 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -351,8 +351,11 @@ int netfs_write_begin(struct netfs_inode *ctx,
>  		ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
>  		if (ret < 0) {
>  			trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
> -			if (ret == -EAGAIN)
> +			if (ret == -EAGAIN) {
> +				folio_unlock(folio);
> +				folio_put(folio);
>  				goto retry;
> +			}
>  			goto error;
>  		}
>  	}

I don't know here... I think it might be better to just expect that when
this function returns an error that the folio has already been unlocked.
Doing it this way will mean that you will lock and unlock the folio a
second time for no reason.

Maybe something like this instead?

diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 42f892c5712e..8ae7b0f4c909 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -353,7 +353,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
                        trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
                        if (ret == -EAGAIN)
                                goto retry;
-                       goto error;
+                       goto error_unlocked;
                }
        }
 
@@ -418,6 +418,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
 error:
        folio_unlock(folio);
        folio_put(folio);
+error_unlocked:
        _leave(" = %d", ret);
        return ret;
 }