| Message ID | 20240821-nfs-6-11-v1-1-ce61f5fc7587@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs: fix bitmap decoder to handle a 3rd word | expand |
On Wed, 2024-08-21 at 08:16 -0400, Jeff Layton wrote: > It only decodes the first two words at this point. Have it decode the > third word as well. Without this, the client doesn't send delegated > timestamps in the CB_GETATTR response. > > Fixes: 43df7110f4a9 ("NFSv4: Add CB_GETATTR support for delegated > attributes") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > Found this while working on the delstid patches for nfsd. > --- > fs/nfs/callback_xdr.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > index 29c49a7e5fe1..246470306172 100644 > --- a/fs/nfs/callback_xdr.c > +++ b/fs/nfs/callback_xdr.c > @@ -118,7 +118,9 @@ static __be32 decode_bitmap(struct xdr_stream > *xdr, uint32_t *bitmap) > if (likely(attrlen > 0)) > bitmap[0] = ntohl(*p++); > if (attrlen > 1) > - bitmap[1] = ntohl(*p); > + bitmap[1] = ntohl(*p++); > + if (attrlen > 2) > + bitmap[2] = ntohl(*p); > return 0; > } > > > --- > base-commit: b311c1b497e51a628aa89e7cb954481e5f9dced2 > change-id: 20240821-nfs-6-11-188bb4e1f1dd > > Best regards, Why do we need this? I'm not really understanding which callback attributes we'd want to return in that range.
On Wed, 2024-08-21 at 14:58 +0000, Trond Myklebust wrote: > On Wed, 2024-08-21 at 08:16 -0400, Jeff Layton wrote: > > It only decodes the first two words at this point. Have it decode the > > third word as well. Without this, the client doesn't send delegated > > timestamps in the CB_GETATTR response. > > > > Fixes: 43df7110f4a9 ("NFSv4: Add CB_GETATTR support for delegated > > attributes") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > Found this while working on the delstid patches for nfsd. > > --- > > fs/nfs/callback_xdr.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > index 29c49a7e5fe1..246470306172 100644 > > --- a/fs/nfs/callback_xdr.c > > +++ b/fs/nfs/callback_xdr.c > > @@ -118,7 +118,9 @@ static __be32 decode_bitmap(struct xdr_stream > > *xdr, uint32_t *bitmap) > > if (likely(attrlen > 0)) > > bitmap[0] = ntohl(*p++); > > if (attrlen > 1) > > - bitmap[1] = ntohl(*p); > > + bitmap[1] = ntohl(*p++); > > + if (attrlen > 2) > > + bitmap[2] = ntohl(*p); > > return 0; > > } > > > > > > --- > > base-commit: b311c1b497e51a628aa89e7cb954481e5f9dced2 > > change-id: 20240821-nfs-6-11-188bb4e1f1dd > > > > Best regards, > > Why do we need this? I'm not really understanding which callback > attributes we'd want to return in that range. > (note that there is a v2 that fixes a potential buffer overrun with this change. We'll want that one) FATTR4_WORD2_TIME_DELEG_ACCESS and FATTR4_WORD2_TIME_DELEG_MODIFY. 43df7110f4a90 added support for those, but the client doesn't see the server's request for them without this change.
On Wed, 2024-08-21 at 11:01 -0400, Jeff Layton wrote: > On Wed, 2024-08-21 at 14:58 +0000, Trond Myklebust wrote: > > On Wed, 2024-08-21 at 08:16 -0400, Jeff Layton wrote: > > > It only decodes the first two words at this point. Have it decode > > > the > > > third word as well. Without this, the client doesn't send > > > delegated > > > timestamps in the CB_GETATTR response. > > > > > > Fixes: 43df7110f4a9 ("NFSv4: Add CB_GETATTR support for delegated > > > attributes") > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > Found this while working on the delstid patches for nfsd. > > > --- > > > fs/nfs/callback_xdr.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > > index 29c49a7e5fe1..246470306172 100644 > > > --- a/fs/nfs/callback_xdr.c > > > +++ b/fs/nfs/callback_xdr.c > > > @@ -118,7 +118,9 @@ static __be32 decode_bitmap(struct xdr_stream > > > *xdr, uint32_t *bitmap) > > > if (likely(attrlen > 0)) > > > bitmap[0] = ntohl(*p++); > > > if (attrlen > 1) > > > - bitmap[1] = ntohl(*p); > > > + bitmap[1] = ntohl(*p++); > > > + if (attrlen > 2) > > > + bitmap[2] = ntohl(*p); > > > return 0; > > > } > > > > > > > > > --- > > > base-commit: b311c1b497e51a628aa89e7cb954481e5f9dced2 > > > change-id: 20240821-nfs-6-11-188bb4e1f1dd > > > > > > Best regards, > > > > Why do we need this? I'm not really understanding which callback > > attributes we'd want to return in that range. > > > > (note that there is a v2 that fixes a potential buffer overrun with > this change. We'll want that one) > > FATTR4_WORD2_TIME_DELEG_ACCESS and FATTR4_WORD2_TIME_DELEG_MODIFY. > 43df7110f4a90 added support for those, but the client doesn't see the > server's request for them without this change. > D'oh! Acked...
On Wed, 2024-08-21 at 15:37 +0000, Trond Myklebust wrote: > On Wed, 2024-08-21 at 11:01 -0400, Jeff Layton wrote: > > On Wed, 2024-08-21 at 14:58 +0000, Trond Myklebust wrote: > > > On Wed, 2024-08-21 at 08:16 -0400, Jeff Layton wrote: > > > > It only decodes the first two words at this point. Have it > > > > decode > > > > the > > > > third word as well. Without this, the client doesn't send > > > > delegated > > > > timestamps in the CB_GETATTR response. > > > > > > > > Fixes: 43df7110f4a9 ("NFSv4: Add CB_GETATTR support for > > > > delegated > > > > attributes") > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > Found this while working on the delstid patches for nfsd. > > > > --- > > > > fs/nfs/callback_xdr.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > > > index 29c49a7e5fe1..246470306172 100644 > > > > --- a/fs/nfs/callback_xdr.c > > > > +++ b/fs/nfs/callback_xdr.c > > > > @@ -118,7 +118,9 @@ static __be32 decode_bitmap(struct > > > > xdr_stream > > > > *xdr, uint32_t *bitmap) > > > > if (likely(attrlen > 0)) > > > > bitmap[0] = ntohl(*p++); > > > > if (attrlen > 1) > > > > - bitmap[1] = ntohl(*p); > > > > + bitmap[1] = ntohl(*p++); > > > > + if (attrlen > 2) > > > > + bitmap[2] = ntohl(*p); > > > > return 0; > > > > } > > > > > > > > > > > > --- > > > > base-commit: b311c1b497e51a628aa89e7cb954481e5f9dced2 > > > > change-id: 20240821-nfs-6-11-188bb4e1f1dd > > > > > > > > Best regards, > > > > > > Why do we need this? I'm not really understanding which callback > > > attributes we'd want to return in that range. > > > > > > > (note that there is a v2 that fixes a potential buffer overrun with > > this change. We'll want that one) > > > > FATTR4_WORD2_TIME_DELEG_ACCESS and FATTR4_WORD2_TIME_DELEG_MODIFY. > > 43df7110f4a90 added support for those, but the client doesn't see > > the > > server's request for them without this change. > > > > D'oh! Acked... > Sorry. I mean Ack for the v2 that also fixes the resulting potential for an array overflow in decode_recallany_args.
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c index 29c49a7e5fe1..246470306172 100644 --- a/fs/nfs/callback_xdr.c +++ b/fs/nfs/callback_xdr.c @@ -118,7 +118,9 @@ static __be32 decode_bitmap(struct xdr_stream *xdr, uint32_t *bitmap) if (likely(attrlen > 0)) bitmap[0] = ntohl(*p++); if (attrlen > 1) - bitmap[1] = ntohl(*p); + bitmap[1] = ntohl(*p++); + if (attrlen > 2) + bitmap[2] = ntohl(*p); return 0; }
It only decodes the first two words at this point. Have it decode the third word as well. Without this, the client doesn't send delegated timestamps in the CB_GETATTR response. Fixes: 43df7110f4a9 ("NFSv4: Add CB_GETATTR support for delegated attributes") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- Found this while working on the delstid patches for nfsd. --- fs/nfs/callback_xdr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- base-commit: b311c1b497e51a628aa89e7cb954481e5f9dced2 change-id: 20240821-nfs-6-11-188bb4e1f1dd Best regards,