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: <172100324023.15471.746980048334211968@noble.neil.brown.name>
Date: Mon, 15 Jul 2024 10:27:20 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Chuck Lever" <chuck.lever@...cle.com>,
 "Olga Kornievskaia" <kolga@...app.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, linux-nfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, "Youzhong Yang" <youzhong@...il.com>,
 "Jeff Layton" <jlayton@...nel.org>
Subject:
 Re: [PATCH] nfsd: remove unneeded EEXIST error check in nfsd_do_file_acquire

On Fri, 12 Jul 2024, Jeff Layton wrote:
> Given that we do the search and insertion while holding the i_lock, I
> don't think it's possible for us to get EEXIST here. Remove this case.

I was going to comment that as rhltable_insert() cannot return -EEXIST
that is an extra reason to discard the check.  But then I looked at the
code an I cannot convince myself that it cannot.
If __rhashtable_insert_fast() finds that tbl->future_tbl is not NULL it
calls rhashtable_insert_slow(), and that seems to fail if the key
already exists.  But it shouldn't for an rhltable, it should just add
the new item to the linked list for that key.

It looks like this has always been broken: adding to an rhltable during
a resize event can cause EEXIST....

Would anyone like to check my work?  I'm surprise that hasn't been
noticed if it is really the case.

NeilBrown


> 
> Cc: Youzhong Yang <youzhong@...il.com>
> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of lease break error")
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> This is replacement for PATCH 1/3 in the series I sent yesterday. I
> think it makes sense to just eliminate this case.
> ---
>  fs/nfsd/filecache.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index f84913691b78..b9dc7c22242c 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1038,8 +1038,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (likely(ret == 0))
>  		goto open_file;
>  
> -	if (ret == -EEXIST)
> -		goto retry;
>  	trace_nfsd_file_insert_err(rqstp, inode, may_flags, ret);
>  	status = nfserr_jukebox;
>  	goto construction_err;
> 
> ---
> base-commit: ec1772c39fa8dd85340b1a02040806377ffbff27
> change-id: 20240711-nfsd-next-c9d17f66e2bd
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@...nel.org>
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ