On Fri, 21 Jul 2023, Jeff Layton wrote:
> Boyang reported tripping the BUG_ON in set_change_info. While we
> couldn't confirm it, one way this could happen would be for nfsd_lookup
> to succeed and then for fh_fill_both_attrs to fail.
>
> This patchset attempts to (sanely) fix this, usually by aborting the
> operation if fetching the pre attributes fails. Post-op attribute fetch
> handling is more difficult to deal with however since we've already done
> the operation, so this has it just fudge the change_info4 if that
> occurs.
I think both v3 and v4 allow a reply that says "the operation was a
success but there are no post-op attrs". With v4 you can say "there is
no change-attr, but here are some other attrs". I think.
Our xdr-encoding doesn't make that easy, but it is just a "simple matter
of coding". If you think it is worth it.
NeilBrown
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> Changes in v2:
> - make fh_fill_*_attrs return an error and have the callers handle it
> - rework of set_change_info, to better handle missing pre/post attrs
>
> ---
> Jeff Layton (2):
> nfsd: handle failure to collect pre/post-op attrs more sanely
> nfsd: remove unsafe BUG_ON from set_change_info
>
> fs/nfsd/nfs3proc.c | 4 +++-
> fs/nfsd/nfs4proc.c | 45 +++++++++++++++++++++++++++++++++------
> fs/nfsd/nfsfh.c | 26 ++++++++++++++---------
> fs/nfsd/nfsfh.h | 6 +++---
> fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++--------------------
> fs/nfsd/xdr4.h | 11 ----------
> 6 files changed, 100 insertions(+), 54 deletions(-)
> ---
> base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
> change-id: 20230720-bz2223560-9c4690a8217b
>
> Best regards,
> --
> Jeff Layton <[email protected]>
>
>
On Fri, Jul 21, 2023 at 07:42:47AM +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > Boyang reported tripping the BUG_ON in set_change_info. While we
> > couldn't confirm it, one way this could happen would be for nfsd_lookup
> > to succeed and then for fh_fill_both_attrs to fail.
> >
> > This patchset attempts to (sanely) fix this, usually by aborting the
> > operation if fetching the pre attributes fails. Post-op attribute fetch
> > handling is more difficult to deal with however since we've already done
> > the operation, so this has it just fudge the change_info4 if that
> > occurs.
>
> I think both v3 and v4 allow a reply that says "the operation was a
> success but there are no post-op attrs". With v4 you can say "there is
> no change-attr, but here are some other attrs". I think.
If the protocols enable NFSD to avoid returning made-up values, I'm
all for it.
> Our xdr-encoding doesn't make that easy, but it is just a "simple matter
> of coding". If you think it is worth it.
>
> NeilBrown
>
>
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > Changes in v2:
> > - make fh_fill_*_attrs return an error and have the callers handle it
> > - rework of set_change_info, to better handle missing pre/post attrs
> >
> > ---
> > Jeff Layton (2):
> > nfsd: handle failure to collect pre/post-op attrs more sanely
> > nfsd: remove unsafe BUG_ON from set_change_info
> >
> > fs/nfsd/nfs3proc.c | 4 +++-
> > fs/nfsd/nfs4proc.c | 45 +++++++++++++++++++++++++++++++++------
> > fs/nfsd/nfsfh.c | 26 ++++++++++++++---------
> > fs/nfsd/nfsfh.h | 6 +++---
> > fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++--------------------
> > fs/nfsd/xdr4.h | 11 ----------
> > 6 files changed, 100 insertions(+), 54 deletions(-)
> > ---
> > base-commit: 070f391ca4d48e1750ee6108eb44f751a9e9448e
> > change-id: 20230720-bz2223560-9c4690a8217b
> >
> > Best regards,
> > --
> > Jeff Layton <[email protected]>
> >
> >
>