1. Patchew
  2. linux
  3. View series

[PATCH] nfs: fix bitmap decoder to handle a 3rd word

Jeff Layton posted 1 patch 1 year, 3 months ago
There is a newer version of this series
fs/nfs/callback_xdr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] nfs: fix bitmap decoder to handle a 3rd word
Posted by Jeff Layton 1 year, 3 months ago 
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,
-- 
Jeff Layton <jlayton@kernel.org>
Re: [PATCH] nfs: fix bitmap decoder to handle a 3rd word
Posted by Trond Myklebust 1 year, 3 months ago 
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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
Re: [PATCH] nfs: fix bitmap decoder to handle a 3rd word
Posted by Jeff Layton 1 year, 3 months ago 
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.

-- 
Jeff Layton <jlayton@kernel.org>
Re: [PATCH] nfs: fix bitmap decoder to handle a 3rd word
Posted by Trond Myklebust 1 year, 3 months ago 
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...

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
Re: [PATCH] nfs: fix bitmap decoder to handle a 3rd word
Posted by Trond Myklebust 1 year, 3 months ago 
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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.