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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
