linux-kernel - Re: [PATCH v4 0/2] nfsd: CB_SEQUENCE error handling fixes and cleanups

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] [day] [month] [year] [list]

Message-ID: <7acd540a-d086-4ae6-b249-8f921e5d5240@oracle.com>
Date: Fri, 7 Feb 2025 10:08:34 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>, "J. Bruce Fields" <bfields@...ldses.org>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/2] nfsd: CB_SEQUENCE error handling fixes and
 cleanups

On 2/7/25 8:45 AM, Jeff Layton wrote:
> Bruce convinced me that the single-threadedness of the workqueue and the
> fact that the RPCs are killed synchronously was enough to ensure that
> the session can't disappear out from under a running callback. Given
> that, I'm breaking these patches out into a separate series that can
> potentially be backported.
> 
> I think this rework makes sense, and I've run these against pynfs,
> fstests, and nfstest, but I'm not sure how well that stresses the
> backchannel error handling. I'd like to put this into linux-next for now
> and see if any problems arise.

I'm good with the proposed code changes. A couple of general comments:

- I plan to put these changes in nfsd-testing first to get some local
  NFS testing experience under our belts before exposing them to the
  broader community of testers.

- nfsd-testing is not automatically merged with linux-next or fs-next.
  These changes would not appear in linux-next until I move them to
  nfsd-next.

- I prefer having the first patch broken into smaller changes to give
  bisection a little more leverage. Eg, the "restart:" and
  "bool = false" changes are fine in a single patch, but change the
  recovery actions for each status code in separate patches, maybe?


> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> Changes in v4:
> - Hold back on session refcounting changes for now and just send CB_SEQUENCE
>   error handling rework.
> - Link to v3: https://lore.kernel.org/r/20250129-nfsd-6-14-v3-0-506e71e39e6b@kernel.org
> 
> Changes in v3:
> - rename cb_session_changed to nfsd4_cb_session_changed
> - rename restart_callback to requeue_callback, and rename need_restart:
>   label to requeue:
> - don't increment seq_nr on -ESERVERFAULT
> - comment cleanups
> - drop client-side rpc patch (will send separately)
> - Link to v2: https://lore.kernel.org/r/20250129-nfsd-6-14-v2-0-2700c92f3e44@kernel.org
> 
> Changes in v2:
> - make nfsd4_session be RCU-freed
> - change code to keep reference to session over callback RPCs
> - rework error handling in nfsd4_cb_sequence_done()
> - move NFSv4.0 handling out of nfsd4_cb_sequence_done()
> - Link to v1: https://lore.kernel.org/r/20250123-nfsd-6-14-v1-0-c1137a4fa2ae@kernel.org
> 
> ---
> Jeff Layton (2):
>       nfsd: overhaul CB_SEQUENCE error handling
>       nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done()
> 
>  fs/nfsd/nfs4callback.c | 107 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 41 deletions(-)
> ---
> base-commit: 50934b1a613cabba2b917879c3e722882b72f628
> change-id: 20250123-nfsd-6-14-b0797e385dc0
> 
> Best regards,


-- 
Chuck Lever