[PATCH] net:mctp: split mctp hdr version to ver and rsvd

[PATCH] net:mctp: split mctp hdr version to ver and rsvd

[wit_yuan]

From: yuanzhaoming <yuanzm2@lenovo.com>

from spec dsp0236_1.2.1.pdf page 26, the mctp header contains the
RSVD(4bit) and Hdr version(4 bit).

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.

Signed-off-by: yuanzhaoming <yuanzm2@lenovo.com>
---
 include/net/mctp.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/mctp.h b/include/net/mctp.h
index e1e0a69afdce..80cc9c63f6ba 100644
--- a/include/net/mctp.h
+++ b/include/net/mctp.h
@@ -14,10 +14,17 @@
 #include <linux/netdevice.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>
+#include <asm/byteorder.h>
 
 /* MCTP packet definitions */
 struct mctp_hdr {
-	u8	ver;
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	u8	ver:4, rsvd: 4;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	u8	rsvd:4, ver: 4;
+#else
+#error	"Please fix <asm/byteorder.h>"
+#endif
 	u8	dest;
 	u8	src;
 	u8	flags_seq_tag;
-- 
2.49.0

Re: [PATCH] net:mctp: split mctp hdr version to ver and rsvd

[Jeremy Kerr]

Hi yuanxhaoming,

> from spec dsp0236_1.2.1.pdf page 26, the mctp header contains the
> RSVD(4bit) and Hdr version(4 bit).
> 
> 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.
> 
> Signed-off-by: yuanzhaoming <yuanzm2@lenovo.com>
> ---
>  include/net/mctp.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/mctp.h b/include/net/mctp.h
> index e1e0a69afdce..80cc9c63f6ba 100644
> --- a/include/net/mctp.h
> +++ b/include/net/mctp.h
> @@ -14,10 +14,17 @@
>  #include <linux/netdevice.h>
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
> +#include <asm/byteorder.h>
>  
>  /* MCTP packet definitions */
>  struct mctp_hdr {
> -       u8      ver;
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +       u8      ver:4, rsvd: 4;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +       u8      rsvd:4, ver: 4;
> +#else
> +#error "Please fix <asm/byteorder.h>"
> +#endif

I would strongly prefer that we do not use C bitfields for a wire
format. The existing flags_seq_tag member contains three fields, which
we use with a couple of helpers to extract the flag, sequence number or
tag values - please follow that convention if we need changes here.

Also, this introduces a few subtle bugs in that we are no longer setting
the reserved bits to zero when preparing an outgoing TX packet header.

What is the underlying issue are you fixing here? Are you seeing a peer
that is sending us packets with bits set in the reserved field?

(if so, that would also be handy information to include in the commit
message)

> From: yuanzhaoming <yuanzm2@lenovo.com>

Is this the preferred format of your name? These are generally in
a full-name format, or a known identity. There's no particular issue
with what you're using there, if that's what you prefer.

Cheers,


Jeremy

(no subject)

[wit_yuan]

thanks for you reply, jeremy.

I will change the identity.

I also modify the optimized code.

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

[wit_yuan]

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).

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.

one type hba card will set pcie vdm header Pad Len the same with RSVD
bit, this will not work on mctp kernel solution.

Signed-off-by: Yuan Zhaoming <yuanzm2@lenovo.com>

---
 include/net/mctp.h | 1 +
 net/mctp/route.c   | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/mctp.h b/include/net/mctp.h
index f481007a6..1392fafe7 100644
--- a/include/net/mctp.h
+++ b/include/net/mctp.h
@@ -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)
 
 #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)
 		goto out;
 
 	flags = mh->flags_seq_tag & (MCTP_HDR_FLAG_SOM | MCTP_HDR_FLAG_EOM);
@@ -1345,7 +1345,7 @@ static int mctp_pkttype_receive(struct sk_buff *skb, struct net_device *dev,
 
 	/* We have enough for a header; decode and route */
 	mh = mctp_hdr(skb);
-	if (mh->ver < MCTP_VER_MIN || mh->ver > MCTP_VER_MAX)
+	if (((mh->ver & MCTP_HDR_VER_MASK)) < MCTP_VER_MIN || (mh->ver & MCTP_HDR_VER_MASK) > MCTP_VER_MAX)
 		goto err_drop;
 
 	/* source must be valid unicast or null; drop reserved ranges and
-- 
2.43.0

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

[Jeremy Kerr]

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