Re: [PATCH next-next v2] net:mctp: fix setting mctp hdr ver reserved field cause data dropped

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi,

Looks like a better approach, thanks for the improvements, including the
net-next patch prefix.

I would suggest reading up on the contribution guidelines too:

 https://docs.kernel.org/process/maintainer-netdev.html

(in this case, the 24 hour spacing between revisions).

> Subject: [PATCH next-next v2] net:mctp: fix setting mctp hdr ver reserved
>   field cause  data dropped

Add a space between the 'net:mctp:'. That should be 'net: mctp: [...]'

And perhaps a tweak to the wording, giving:

  net: mctp: don't require received header reserved bits to be zero

> From: Yuan Zhaoming <yuanzm2@lenovo.com>
> 
> from spec dsp0236_1.2.1.pdf page 26, the mctp header contains the
> RSVD(4bit) and Hdr version(4 bit).

Some wording things there - It would be more clear to reference the spec
by name, rather than by filename, giving something like:

  From the MCTP Base specification (DSP0236 v1.2.1), the first byte of
  the MCTP header contains a 4 bit reserved field, and 4 bit version.

Then, explain the current issue:

  On our current receive path, we require those 4 reserved bits to be
  zero, but at least one device is non-conformant, and may set these
  reserved bits.

[If you can name the specific device, that will certainly help others
with future debugging. I assume the device is currently public?]

Then, maybe some rationale about the change, and an overview of the
implementation:

  DSP0236 states that the reserved bits must be written as zero, and
  ignored when read. While the device might not conform to the former,
  we should accept these message to conform to the latter.

  Relax our check on the MCTP version byte to allow non-zero bits in the
  reserved field.

  Signed-off-by: ...

> mctp_pkttype_receive invoke mctp_hdr, and get mh->ver whole byte 
> compare the MCTP_VER_MIN, MCTP_VER_MAX. the reserver bits may be by 
> misleading used.

This is no longer relevant for the new revision.

> @@ -35,6 +35,7 @@ struct mctp_hdr {
>  #define MCTP_HDR_SEQ_MASK      GENMASK(1, 0)
>  #define MCTP_HDR_TAG_SHIFT     0
>  #define MCTP_HDR_TAG_MASK      GENMASK(2, 0)
> +#define MCTP_HDR_VER_MASK      GENMASK(3, 0)

This section of #defines is commented to represent the flags/seq/tag
field. I would prefer to move this definition above that comment.

>  
>  #define MCTP_INITIAL_DEFAULT_NET       1
>  
> diff --git a/net/mctp/route.c b/net/mctp/route.c
> index 56c441e90..aa0e89d67 100644
> --- a/net/mctp/route.c
> +++ b/net/mctp/route.c
> @@ -464,7 +464,7 @@ static int mctp_dst_input(struct mctp_dst *dst, struct sk_buff *skb)
>         netid = mctp_cb(skb)->net;
>         skb_pull(skb, sizeof(struct mctp_hdr));
>  
> -       if (mh->ver != 1)
> +       if (((mh->ver & MCTP_HDR_VER_MASK)) < MCTP_VER_MIN || (mh->ver & MCTP_HDR_VER_MASK) > MCTP_VER_MAX)

Use a temporary for the version, rather than repeating the mask
operation. This makes the conditional much easier to read:

    if (ver < MCTP_VER_MIN || ver > MCTP_VER_MAX)

Cheers,


Jeremy