| Message ID | 20240828174001.322745-15-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Possible NFSD COPY clean-ups | expand |
On Wed, Aug 28, 2024 at 1:40 PM <cel@kernel.org> wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > NFSD removes COPY callback stateids after once lease period. This > practice keeps the list of callback stateids limited to prevent a > DoS possibility, but doesn't comply with the spirit of RFC 7862 > Section 4.8, which says: > > > A copy offload stateid will be valid until either (A) the client or > > server restarts or (B) the client returns the resource by issuing an > > OFFLOAD_CANCEL operation or the client replies to a CB_OFFLOAD > > operation. > > Note there are no BCP 14 compliance keywords in this text, so NFSD > is free to ignore this stateid lifetime guideline without becoming > non-compliant. > > Nevertheless, this behavior variance should be explicitly documented > in the code. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4state.c | 36 +++++++++++++++++++++++++----------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index aaebc60cc77c..437b94beb115 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -6324,6 +6324,29 @@ static void nfsd4_ssc_expire_umount(struct nfsd_net *nn) > } > #endif > > +/* > + * RFC 7862 Section 4.8 says that, if the client hasn't replied to a > + * CB_OFFLOAD, that COPY callback stateid will live until the client or > + * server restarts. To prevent a DoS resulting from a pile of these > + * stateids accruing over time, NFSD purges them after one lease period. > + */ I don't believe this is correct documentation for this piece of code. There are two kinds of stateids in the copy offload world: one is issued by the source server "cnr_stateid" in the response of the COPY_NOTIFY and given to be client (to be given to the destination server to use for the read) and those are the ones kept in the knfsd's s2s_cp_stateids list and then cleaned up by the laundry thread when copy isn't cleaned up on the source server. The text from the RFC quoted here is for copy's callback stateids. In the current implementation, we don't keep callback stateids around past when the async copy is done. I agree that needs to be changed for OFFLOAD_STATUS op and then we can add the wording of how long we are keeping those. > +static void nfs4_launder_cpntf_statelist(struct nfsd_net *nn, > + struct laundry_time *lt) > +{ > + struct nfs4_cpntf_state *cps; > + copy_stateid_t *cps_t; > + int i; > + > + spin_lock(&nn->s2s_cp_lock); > + idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { > + cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); > + if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && > + state_expired(lt, cps->cpntf_time)) > + _free_cpntf_state_locked(nn, cps); > + } > + spin_unlock(&nn->s2s_cp_lock); > +} > + > /* Check if any lock belonging to this lockowner has any blockers */ > static bool > nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) > @@ -6495,9 +6518,6 @@ nfs4_laundromat(struct nfsd_net *nn) > .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease, > .new_timeo = nn->nfsd4_lease > }; > - struct nfs4_cpntf_state *cps; > - copy_stateid_t *cps_t; > - int i; > > if (clients_still_reclaiming(nn)) { > lt.new_timeo = 0; > @@ -6505,14 +6525,8 @@ nfs4_laundromat(struct nfsd_net *nn) > } > nfsd4_end_grace(nn); > > - spin_lock(&nn->s2s_cp_lock); > - idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { > - cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); > - if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && > - state_expired(<, cps->cpntf_time)) > - _free_cpntf_state_locked(nn, cps); > - } > - spin_unlock(&nn->s2s_cp_lock); > + nfs4_launder_cpntf_statelist(nn, <); > + > nfs4_get_client_reaplist(nn, &reaplist, <); > nfs4_process_client_reaplist(&reaplist); > > -- > 2.46.0 > >
> On Aug 28, 2024, at 6:49 PM, Olga Kornievskaia <aglo@umich.edu> wrote: > > On Wed, Aug 28, 2024 at 1:40 PM <cel@kernel.org> wrote: >> >> From: Chuck Lever <chuck.lever@oracle.com> >> >> NFSD removes COPY callback stateids after once lease period. This >> practice keeps the list of callback stateids limited to prevent a >> DoS possibility, but doesn't comply with the spirit of RFC 7862 >> Section 4.8, which says: >> >>> A copy offload stateid will be valid until either (A) the client or >>> server restarts or (B) the client returns the resource by issuing an >>> OFFLOAD_CANCEL operation or the client replies to a CB_OFFLOAD >>> operation. >> >> Note there are no BCP 14 compliance keywords in this text, so NFSD >> is free to ignore this stateid lifetime guideline without becoming >> non-compliant. >> >> Nevertheless, this behavior variance should be explicitly documented >> in the code. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> fs/nfsd/nfs4state.c | 36 +++++++++++++++++++++++++----------- >> 1 file changed, 25 insertions(+), 11 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index aaebc60cc77c..437b94beb115 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -6324,6 +6324,29 @@ static void nfsd4_ssc_expire_umount(struct nfsd_net *nn) >> } >> #endif >> >> +/* >> + * RFC 7862 Section 4.8 says that, if the client hasn't replied to a >> + * CB_OFFLOAD, that COPY callback stateid will live until the client or >> + * server restarts. To prevent a DoS resulting from a pile of these >> + * stateids accruing over time, NFSD purges them after one lease period. >> + */ > > I don't believe this is correct documentation for this piece of code. > There are two kinds of stateids in the copy offload world: one is > issued by the source server "cnr_stateid" in the response of the > COPY_NOTIFY and given to be client (to be given to the destination > server to use for the read) and those are the ones kept in the > knfsd's s2s_cp_stateids list and then cleaned up by the laundry thread > when copy isn't cleaned up on the source server. Got it. That detail was not included in what you mentioned to me before ;-) Either I can drop this patch, or please suggest a corrected text and I will replace the comment. > The text from the RFC > quoted here is for copy's callback stateids. I'll look for something more relevant, if only for my own edification. > In the current > implementation, we don't keep callback stateids around past when the > async copy is done. I agree that needs to be changed for > OFFLOAD_STATUS op and then we can add the wording of how long we are > keeping those. Yep, as we discussed yesterday. >> +static void nfs4_launder_cpntf_statelist(struct nfsd_net *nn, >> + struct laundry_time *lt) >> +{ >> + struct nfs4_cpntf_state *cps; >> + copy_stateid_t *cps_t; >> + int i; >> + >> + spin_lock(&nn->s2s_cp_lock); >> + idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { >> + cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); >> + if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && >> + state_expired(lt, cps->cpntf_time)) >> + _free_cpntf_state_locked(nn, cps); >> + } >> + spin_unlock(&nn->s2s_cp_lock); >> +} >> + >> /* Check if any lock belonging to this lockowner has any blockers */ >> static bool >> nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) >> @@ -6495,9 +6518,6 @@ nfs4_laundromat(struct nfsd_net *nn) >> .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease, >> .new_timeo = nn->nfsd4_lease >> }; >> - struct nfs4_cpntf_state *cps; >> - copy_stateid_t *cps_t; >> - int i; >> >> if (clients_still_reclaiming(nn)) { >> lt.new_timeo = 0; >> @@ -6505,14 +6525,8 @@ nfs4_laundromat(struct nfsd_net *nn) >> } >> nfsd4_end_grace(nn); >> >> - spin_lock(&nn->s2s_cp_lock); >> - idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { >> - cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); >> - if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && >> - state_expired(<, cps->cpntf_time)) >> - _free_cpntf_state_locked(nn, cps); >> - } >> - spin_unlock(&nn->s2s_cp_lock); >> + nfs4_launder_cpntf_statelist(nn, <); >> + >> nfs4_get_client_reaplist(nn, &reaplist, <); >> nfs4_process_client_reaplist(&reaplist); >> >> -- >> 2.46.0 -- Chuck Lever
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index aaebc60cc77c..437b94beb115 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6324,6 +6324,29 @@ static void nfsd4_ssc_expire_umount(struct nfsd_net *nn) } #endif +/* + * RFC 7862 Section 4.8 says that, if the client hasn't replied to a + * CB_OFFLOAD, that COPY callback stateid will live until the client or + * server restarts. To prevent a DoS resulting from a pile of these + * stateids accruing over time, NFSD purges them after one lease period. + */ +static void nfs4_launder_cpntf_statelist(struct nfsd_net *nn, + struct laundry_time *lt) +{ + struct nfs4_cpntf_state *cps; + copy_stateid_t *cps_t; + int i; + + spin_lock(&nn->s2s_cp_lock); + idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { + cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); + if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && + state_expired(lt, cps->cpntf_time)) + _free_cpntf_state_locked(nn, cps); + } + spin_unlock(&nn->s2s_cp_lock); +} + /* Check if any lock belonging to this lockowner has any blockers */ static bool nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) @@ -6495,9 +6518,6 @@ nfs4_laundromat(struct nfsd_net *nn) .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease, .new_timeo = nn->nfsd4_lease }; - struct nfs4_cpntf_state *cps; - copy_stateid_t *cps_t; - int i; if (clients_still_reclaiming(nn)) { lt.new_timeo = 0; @@ -6505,14 +6525,8 @@ nfs4_laundromat(struct nfsd_net *nn) } nfsd4_end_grace(nn); - spin_lock(&nn->s2s_cp_lock); - idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) { - cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid); - if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID && - state_expired(<, cps->cpntf_time)) - _free_cpntf_state_locked(nn, cps); - } - spin_unlock(&nn->s2s_cp_lock); + nfs4_launder_cpntf_statelist(nn, <); + nfs4_get_client_reaplist(nn, &reaplist, <); nfs4_process_client_reaplist(&reaplist);