diff mbox

[5/9] NFSv4: Clean up encode_attrs

Message ID 20180320210313.94429-6-trond.myklebust@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust March 20, 2018, 9:03 p.m. UTC 
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs4xdr.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

Comments

Dan Aloni May 17, 2022, 5 p.m. UTC | #1 
Hi Trond,

Wireshark claims that this commit broke encoding of SETATTR (see below).

Is Wireshark correct in reference to the RFC?

Frame 56: 274 bytes on wire (2192 bits), 274 bytes captured (2192 bits)
Ethernet II, Src: RealtekU_d0:d8:ff (52:54:00:d0:d8:ff), Dst: RealtekU_7a:80:63 (52:54:00:7a:80:63)
Internet Protocol Version 4, Src: 192.168.40.1, Dst: 192.168.40.11
Transmission Control Protocol, Src Port: 999, Dst Port: 2049, Seq: 4325, Ack: 4489, Len: 208
Remote Procedure Call, Type:Call XID:0x3725a760
Network File System, Ops(4): SEQUENCE, PUTFH, SETATTR, GETATTR
    [Program Version: 4]
    [V4 Procedure: COMPOUND (1)]
    Tag: <EMPTY>
    minorversion: 1
    Operations (count: 4): SEQUENCE, PUTFH, SETATTR, GETATTR
        Opcode: SEQUENCE (53)
        Opcode: PUTFH (22)
        Opcode: SETATTR (34)
            StateID
                [StateID Hash: 0xafa9]
                StateID seqid: 0
                StateID Other: 000000000000000000000000
                [StateID Other hash: 0x60de]
            [Expert Info (Warning/Protocol): Per RFCs 3530 and 5661 an attribute mask is required but was not provided.]
                [Per RFCs 3530 and 5661 an attribute mask is required but was not provided.]
                [Severity level: Warning]
                [Group: Protocol]
        Opcode: GETATTR (9)
    [Main Opcode: SETATTR (34)]

0000  52 54 00 7a 80 63 52 54 00 d0 d8 ff 08 00 45 00   RT.z.cRT......E.
0010  01 04 a6 16 40 00 40 06 c2 80 c0 a8 28 01 c0 a8   ....@.@.....(...
0020  28 0b 03 e7 08 01 14 fe a6 02 de f4 09 10 80 18   (...............
0030  01 7b d2 53 00 00 01 01 08 0a 68 ba 83 03 e9 ee   .{.S......h.....
0040  7c 0b 80 00 00 cc 37 25 a7 60 00 00 00 00 00 00   |.....7%.`......
0050  00 02 00 01 86 a3 00 00 00 04 00 00 00 01 00 00   ................
0060  00 01 00 00 00 30 00 41 88 3d 00 00 00 16 63 6c   .....0.A.=....cl
0070  69 65 6e 74 2e 6e 66 73 2d 74 65 73 74 69 6e 67   ient.nfs-testing
0080  2e 63 6f 6d 00 00 00 00 00 00 00 00 00 00 00 00   .com............
0090  00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00   ................
00a0  00 00 00 00 00 01 00 00 00 04 00 00 00 35 37 00   .............57.
00b0  00 00 00 a0 00 00 36 00 00 00 00 a0 00 00 00 00   ......6.........
00c0  00 14 00 00 00 00 00 00 00 00 00 00 00 01 00 00   ................
00d0  00 16 00 00 00 10 00 00 69 8d 44 d8 0e 34 0f 7d   ........i.D..4.}
00e0  00 00 00 00 00 00 00 00 00 22 00 00 00 00 00 00   ........."......
00f0  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00   ................
0100  00 00 00 00 00 09 00 00 00 02 00 10 01 1a 00 30   ...............0
0110  a2 3a                                             .:

On 2018-03-20 17:03:09, Trond Myklebust wrote:
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4xdr.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 80c5b519fd6a..3d088230c975 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1052,9 +1052,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
>  	int owner_namelen = 0;
>  	int owner_grouplen = 0;
>  	__be32 *p;
> -	unsigned i;
>  	uint32_t len = 0;
> -	uint32_t bmval_len;
>  	uint32_t bmval[3] = { 0 };
>  
>  	/*
> @@ -1123,19 +1121,8 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
>  		bmval[2] |= FATTR4_WORD2_SECURITY_LABEL;
>  	}
>  
> -	if (bmval[2] != 0)
> -		bmval_len = 3;
> -	else if (bmval[1] != 0)
> -		bmval_len = 2;
> -	else
> -		bmval_len = 1;
> -
> -	p = reserve_space(xdr, 4 + (bmval_len << 2) + 4 + len);
> -
> -	*p++ = cpu_to_be32(bmval_len);
> -	for (i = 0; i < bmval_len; i++)
> -		*p++ = cpu_to_be32(bmval[i]);
> -	*p++ = cpu_to_be32(len);
> +	xdr_encode_bitmap4(xdr, bmval, ARRAY_SIZE(bmval));
> +	xdr_stream_encode_opaque_inline(xdr, (void **)&p, len);
>  
>  	if (bmval[0] & FATTR4_WORD0_SIZE)
>  		p = xdr_encode_hyper(p, iap->ia_size);
> -- 
> 2.14.3
>
Trond Myklebust May 17, 2022, 6:20 p.m. UTC | #2 
On Tue, 2022-05-17 at 20:00 +0300, Dan Aloni wrote:
> Hi Trond,
> 
> Wireshark claims that this commit broke encoding of SETATTR (see
> below).
> 
> Is Wireshark correct in reference to the RFC?
> 
> Frame 56: 274 bytes on wire (2192 bits), 274 bytes captured (2192
> bits)
> Ethernet II, Src: RealtekU_d0:d8:ff (52:54:00:d0:d8:ff), Dst:
> RealtekU_7a:80:63 (52:54:00:7a:80:63)
> Internet Protocol Version 4, Src: 192.168.40.1, Dst: 192.168.40.11
> Transmission Control Protocol, Src Port: 999, Dst Port: 2049, Seq:
> 4325, Ack: 4489, Len: 208
> Remote Procedure Call, Type:Call XID:0x3725a760
> Network File System, Ops(4): SEQUENCE, PUTFH, SETATTR, GETATTR
>     [Program Version: 4]
>     [V4 Procedure: COMPOUND (1)]
>     Tag: <EMPTY>
>     minorversion: 1
>     Operations (count: 4): SEQUENCE, PUTFH, SETATTR, GETATTR
>         Opcode: SEQUENCE (53)
>         Opcode: PUTFH (22)
>         Opcode: SETATTR (34)
>             StateID
>                 [StateID Hash: 0xafa9]
>                 StateID seqid: 0
>                 StateID Other: 000000000000000000000000
>                 [StateID Other hash: 0x60de]
>             [Expert Info (Warning/Protocol): Per RFCs 3530 and 5661
> an attribute mask is required but was not provided.]
>                 [Per RFCs 3530 and 5661 an attribute mask is required
> but was not provided.]
>                 [Severity level: Warning]
>                 [Group: Protocol]
>         Opcode: GETATTR (9)
>     [Main Opcode: SETATTR (34)]
> 
> 0000  52 54 00 7a 80 63 52 54 00 d0 d8 ff 08 00 45 00  
> RT.z.cRT......E.
> 0010  01 04 a6 16 40 00 40 06 c2 80 c0 a8 28 01 c0 a8  
> ....@.@.....(...
> 0020  28 0b 03 e7 08 01 14 fe a6 02 de f4 09 10 80 18  
> (...............
> 0030  01 7b d2 53 00 00 01 01 08 0a 68 ba 83 03 e9 ee  
> .{.S......h.....
> 0040  7c 0b 80 00 00 cc 37 25 a7 60 00 00 00 00 00 00  
> |.....7%.`......
> 0050  00 02 00 01 86 a3 00 00 00 04 00 00 00 01 00 00  
> ................
> 0060  00 01 00 00 00 30 00 41 88 3d 00 00 00 16 63 6c  
> .....0.A.=....cl
> 0070  69 65 6e 74 2e 6e 66 73 2d 74 65 73 74 69 6e 67   ient.nfs-
> testing
> 0080  2e 63 6f 6d 00 00 00 00 00 00 00 00 00 00 00 00  
> .com............
> 0090  00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
> ................
> 00a0  00 00 00 00 00 01 00 00 00 04 00 00 00 35 37 00  
> .............57.
> 00b0  00 00 00 a0 00 00 36 00 00 00 00 a0 00 00 00 00  
> ......6.........
> 00c0  00 14 00 00 00 00 00 00 00 00 00 00 00 01 00 00  
> ................
> 00d0  00 16 00 00 00 10 00 00 69 8d 44 d8 0e 34 0f 7d  
> ........i.D..4.}
> 00e0  00 00 00 00 00 00 00 00 00 22 00 00 00 00 00 00  
> ........."......
> 00f0  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
> ................
> 0100  00 00 00 00 00 09 00 00 00 02 00 10 01 1a 00 30  
> ...............0
> 0110  a2 3a                                             .:
> 
> On 2018-03-20 17:03:09, Trond Myklebust wrote:
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > ---
> >  fs/nfs/nfs4xdr.c | 17 ++---------------
> >  1 file changed, 2 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > index 80c5b519fd6a..3d088230c975 100644
> > --- a/fs/nfs/nfs4xdr.c
> > +++ b/fs/nfs/nfs4xdr.c
> > @@ -1052,9 +1052,7 @@ static void encode_attrs(struct xdr_stream
> > *xdr, const struct iattr *iap,
> >         int owner_namelen = 0;
> >         int owner_grouplen = 0;
> >         __be32 *p;
> > -       unsigned i;
> >         uint32_t len = 0;
> > -       uint32_t bmval_len;
> >         uint32_t bmval[3] = { 0 };
> >  
> >         /*
> > @@ -1123,19 +1121,8 @@ static void encode_attrs(struct xdr_stream
> > *xdr, const struct iattr *iap,
> >                 bmval[2] |= FATTR4_WORD2_SECURITY_LABEL;
> >         }
> >  
> > -       if (bmval[2] != 0)
> > -               bmval_len = 3;
> > -       else if (bmval[1] != 0)
> > -               bmval_len = 2;
> > -       else
> > -               bmval_len = 1;
> > -
> > -       p = reserve_space(xdr, 4 + (bmval_len << 2) + 4 + len);
> > -
> > -       *p++ = cpu_to_be32(bmval_len);
> > -       for (i = 0; i < bmval_len; i++)
> > -               *p++ = cpu_to_be32(bmval[i]);
> > -       *p++ = cpu_to_be32(len);
> > +       xdr_encode_bitmap4(xdr, bmval, ARRAY_SIZE(bmval));
> > +       xdr_stream_encode_opaque_inline(xdr, (void **)&p, len);
> >  
> >         if (bmval[0] & FATTR4_WORD0_SIZE)
> >                 p = xdr_encode_hyper(p, iap->ia_size);
> > -- 
> > 2.14.3
> > 
> 

Is the bitmap perhaps empty? The new code will just make that a zero
length array. I'm not sure if wireshark would deal correctly with that.

We shouldn't be trying to send a SETATTR with no attributes, but it is
possible that the server just doesn't support the attributes that we're
trying to set.
diff mbox

Patch

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 80c5b519fd6a..3d088230c975 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1052,9 +1052,7 @@  static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 	int owner_namelen = 0;
 	int owner_grouplen = 0;
 	__be32 *p;
-	unsigned i;
 	uint32_t len = 0;
-	uint32_t bmval_len;
 	uint32_t bmval[3] = { 0 };
 
 	/*
@@ -1123,19 +1121,8 @@  static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 		bmval[2] |= FATTR4_WORD2_SECURITY_LABEL;
 	}
 
-	if (bmval[2] != 0)
-		bmval_len = 3;
-	else if (bmval[1] != 0)
-		bmval_len = 2;
-	else
-		bmval_len = 1;
-
-	p = reserve_space(xdr, 4 + (bmval_len << 2) + 4 + len);
-
-	*p++ = cpu_to_be32(bmval_len);
-	for (i = 0; i < bmval_len; i++)
-		*p++ = cpu_to_be32(bmval[i]);
-	*p++ = cpu_to_be32(len);
+	xdr_encode_bitmap4(xdr, bmval, ARRAY_SIZE(bmval));
+	xdr_stream_encode_opaque_inline(xdr, (void **)&p, len);
 
 	if (bmval[0] & FATTR4_WORD0_SIZE)
 		p = xdr_encode_hyper(p, iap->ia_size);