[PATCH net-next] erspan: set bso bit based on mirrored packet's len

[PATCH net-next] erspan: set bso bit based on mirrored packet's len

[William Tu]

Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
handled.  BSO has 4 possible values:
  00 --> Good frame with no error, or unknown integrity
  11 --> Payload is a Bad Frame with CRC or Alignment Error
  01 --> Payload is a Short Frame
  10 --> Payload is an Oversized Frame

Based the short/oversized definitions in RFC1757, the patch sets
the bso bit based on the mirrored packet's size.

Reported-by: Xiaoyan Jin <xiaoyanj@vmware.com>
Signed-off-by: William Tu <u9012063@gmail.com>
---
 include/net/erspan.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/net/erspan.h b/include/net/erspan.h
index d044aa60cc76..5eb95f78ad45 100644
--- a/include/net/erspan.h
+++ b/include/net/erspan.h
@@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
 	return htonl((u32)h_usecs);
 }
 
+/* ERSPAN BSO (Bad/Short/Oversized)
+ *   00b --> Good frame with no error, or unknown integrity
+ *   01b --> Payload is a Short Frame
+ *   10b --> Payload is an Oversized Frame
+ *   11b --> Payload is a Bad Frame with CRC or Alignment Error
+ */
+enum erspan_bso {
+	BSO_NOERROR,
+	BSO_SHORT,
+	BSO_OVERSIZED,
+	BSO_BAD,
+};
+
+static inline u8 erspan_detect_bso(struct sk_buff *skb)
+{
+	if (skb->len < ETH_ZLEN)
+		return BSO_SHORT;
+
+	if (skb->len > ETH_FRAME_LEN)
+		return BSO_OVERSIZED;
+
+	return BSO_NOERROR;
+}
+
 static inline void erspan_build_header_v2(struct sk_buff *skb,
 					  u32 id, u8 direction, u16 hwid,
 					  bool truncate, bool is_ipv4)
@@ -248,6 +272,7 @@ static inline void erspan_build_header_v2(struct sk_buff *skb,
 		vlan_tci = ntohs(qp->tci);
 	}
 
+	bso = erspan_detect_bso(skb);
 	skb_push(skb, sizeof(*ershdr) + ERSPAN_V2_MDSIZE);
 	ershdr = (struct erspan_base_hdr *)skb->data;
 	memset(ershdr, 0, sizeof(*ershdr) + ERSPAN_V2_MDSIZE);
-- 
2.7.4

Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's len

[Tobin C. Harding]

On Mon, May 14, 2018 at 04:54:36PM -0700, William Tu wrote:
> Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
> handled.  BSO has 4 possible values:
>   00 --> Good frame with no error, or unknown integrity
>   11 --> Payload is a Bad Frame with CRC or Alignment Error
>   01 --> Payload is a Short Frame
>   10 --> Payload is an Oversized Frame
> 
> Based the short/oversized definitions in RFC1757, the patch sets
> the bso bit based on the mirrored packet's size.
> 
> Reported-by: Xiaoyan Jin <xiaoyanj@vmware.com>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  include/net/erspan.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/include/net/erspan.h b/include/net/erspan.h
> index d044aa60cc76..5eb95f78ad45 100644
> --- a/include/net/erspan.h
> +++ b/include/net/erspan.h
> @@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
>  	return htonl((u32)h_usecs);
>  }
>  
> +/* ERSPAN BSO (Bad/Short/Oversized)
> + *   00b --> Good frame with no error, or unknown integrity
> + *   01b --> Payload is a Short Frame
> + *   10b --> Payload is an Oversized Frame
> + *   11b --> Payload is a Bad Frame with CRC or Alignment Error
> + */
> +enum erspan_bso {
> +	BSO_NOERROR,
> +	BSO_SHORT,
> +	BSO_OVERSIZED,
> +	BSO_BAD,
> +};

If we are relying on the values perhaps this would be clearer

	BSO_NOERROR	= 0x00,
	BSO_SHORT 	= 0x01,
	BSO_OVERSIZED 	= 0x02,
	BSO_BAD 	= 0x03,

> +
> +static inline u8 erspan_detect_bso(struct sk_buff *skb)
> +{
> +	if (skb->len < ETH_ZLEN)
> +		return BSO_SHORT;
> +
> +	if (skb->len > ETH_FRAME_LEN)
> +		return BSO_OVERSIZED;
> +
> +	return BSO_NOERROR;
> +}

Without having much contextual knowledge around this patch; should we be
doing some check on CRC or alignment (at some stage)?  Having BSO_BAD
seems to imply so? 


Hope this helps,
Tobin.

Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's len

[William Tu]

On Mon, May 14, 2018 at 10:33 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
> On Mon, May 14, 2018 at 04:54:36PM -0700, William Tu wrote:
>> Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
>> handled.  BSO has 4 possible values:
>>   00 --> Good frame with no error, or unknown integrity
>>   11 --> Payload is a Bad Frame with CRC or Alignment Error
>>   01 --> Payload is a Short Frame
>>   10 --> Payload is an Oversized Frame
>>
>> Based the short/oversized definitions in RFC1757, the patch sets
>> the bso bit based on the mirrored packet's size.
>>
>> Reported-by: Xiaoyan Jin <xiaoyanj@vmware.com>
>> Signed-off-by: William Tu <u9012063@gmail.com>
>> ---
>>  include/net/erspan.h | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/include/net/erspan.h b/include/net/erspan.h
>> index d044aa60cc76..5eb95f78ad45 100644
>> --- a/include/net/erspan.h
>> +++ b/include/net/erspan.h
>> @@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
>>       return htonl((u32)h_usecs);
>>  }
>>
>> +/* ERSPAN BSO (Bad/Short/Oversized)
>> + *   00b --> Good frame with no error, or unknown integrity
>> + *   01b --> Payload is a Short Frame
>> + *   10b --> Payload is an Oversized Frame
>> + *   11b --> Payload is a Bad Frame with CRC or Alignment Error
>> + */
>> +enum erspan_bso {
>> +     BSO_NOERROR,
>> +     BSO_SHORT,
>> +     BSO_OVERSIZED,
>> +     BSO_BAD,
>> +};
>
> If we are relying on the values perhaps this would be clearer
>
>         BSO_NOERROR     = 0x00,
>         BSO_SHORT       = 0x01,
>         BSO_OVERSIZED   = 0x02,
>         BSO_BAD         = 0x03,
>

Yes, thanks. I will change in v2.

>> +
>> +static inline u8 erspan_detect_bso(struct sk_buff *skb)
>> +{
>> +     if (skb->len < ETH_ZLEN)
>> +             return BSO_SHORT;
>> +
>> +     if (skb->len > ETH_FRAME_LEN)
>> +             return BSO_OVERSIZED;
>> +
>> +     return BSO_NOERROR;
>> +}
>
> Without having much contextual knowledge around this patch; should we be
> doing some check on CRC or alignment (at some stage)?  Having BSO_BAD
> seems to imply so?
>

The definition of BSO_BAD:
etherStatsCRCAlignErrors OBJECT-TYPE
              SYNTAX Counter
              ACCESS read-only
              STATUS mandatory
              DESCRIPTION
                  "The total number of packets received that
                  had a length (excluding framing bits, but
                  including FCS octets) of between 64 and 1518
                  octets, inclusive, but but had either a bad
                  Frame Check Sequence (FCS) with an integral
                  number of octets (FCS Error) or a bad FCS with
                  a non-integral number of octets (Alignment Error)."

But I don't know how to check CRC error at this code point.
Isn't it done by the NIC hardware?

Thanks for your review!
William

Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's len

[Tobin C. Harding]

On Wed, May 16, 2018 at 07:05:34AM -0700, William Tu wrote:
> On Mon, May 14, 2018 at 10:33 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
> > On Mon, May 14, 2018 at 04:54:36PM -0700, William Tu wrote:
> >> Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
> >> handled.  BSO has 4 possible values:
> >>   00 --> Good frame with no error, or unknown integrity
> >>   11 --> Payload is a Bad Frame with CRC or Alignment Error
> >>   01 --> Payload is a Short Frame
> >>   10 --> Payload is an Oversized Frame
> >>
> >> Based the short/oversized definitions in RFC1757, the patch sets
> >> the bso bit based on the mirrored packet's size.
> >>
> >> Reported-by: Xiaoyan Jin <xiaoyanj@vmware.com>
> >> Signed-off-by: William Tu <u9012063@gmail.com>
> >> ---
> >>  include/net/erspan.h | 25 +++++++++++++++++++++++++
> >>  1 file changed, 25 insertions(+)
> >>
> >> diff --git a/include/net/erspan.h b/include/net/erspan.h
> >> index d044aa60cc76..5eb95f78ad45 100644
> >> --- a/include/net/erspan.h
> >> +++ b/include/net/erspan.h
> >> @@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
> >>       return htonl((u32)h_usecs);
> >>  }
> >>
> >> +/* ERSPAN BSO (Bad/Short/Oversized)
> >> + *   00b --> Good frame with no error, or unknown integrity
> >> + *   01b --> Payload is a Short Frame
> >> + *   10b --> Payload is an Oversized Frame
> >> + *   11b --> Payload is a Bad Frame with CRC or Alignment Error
> >> + */
> >> +enum erspan_bso {
> >> +     BSO_NOERROR,
> >> +     BSO_SHORT,
> >> +     BSO_OVERSIZED,
> >> +     BSO_BAD,
> >> +};
> >
> > If we are relying on the values perhaps this would be clearer
> >
> >         BSO_NOERROR     = 0x00,
> >         BSO_SHORT       = 0x01,
> >         BSO_OVERSIZED   = 0x02,
> >         BSO_BAD         = 0x03,
> >
> 
> Yes, thanks. I will change in v2.
> 
> >> +
> >> +static inline u8 erspan_detect_bso(struct sk_buff *skb)
> >> +{
> >> +     if (skb->len < ETH_ZLEN)
> >> +             return BSO_SHORT;
> >> +
> >> +     if (skb->len > ETH_FRAME_LEN)
> >> +             return BSO_OVERSIZED;
> >> +
> >> +     return BSO_NOERROR;
> >> +}
> >
> > Without having much contextual knowledge around this patch; should we be
> > doing some check on CRC or alignment (at some stage)?  Having BSO_BAD
> > seems to imply so?
> >
> 
> The definition of BSO_BAD:
> etherStatsCRCAlignErrors OBJECT-TYPE
>               SYNTAX Counter
>               ACCESS read-only
>               STATUS mandatory
>               DESCRIPTION
>                   "The total number of packets received that
>                   had a length (excluding framing bits, but
>                   including FCS octets) of between 64 and 1518
>                   octets, inclusive, but but had either a bad
>                   Frame Check Sequence (FCS) with an integral
>                   number of octets (FCS Error) or a bad FCS with
>                   a non-integral number of octets (Alignment Error)."
>
> But I don't know how to check CRC error at this code point.
> Isn't it done by the NIC hardware?

I'll just start with; I don't know anything about ERSPAN

	"ERSPAN is a Cisco proprietary feature and is available only to
	Catalyst 6500, 7600, Nexus, and ASR 1000 platforms to date. The
	ASR 1000 supports ERSPAN source (monitoring) only on Fast
	Ethernet, Gigabit Ethernet, and port-channel interfaces."

https://supportforums.cisco.com/t5/network-infrastructure-documents/understanding-span-rspan-and-erspan/ta-p/3144951

I dug around a bit and none of the files that currently import erspan.h
actually use the 'bso' field

$ grep bso $(git grep -l 'erspan\.h')
include/net/erspan.h:	u8 bso = 0; /* Bad/Short/Oversized */
include/net/erspan.h:	ershdr->en = bso;
net/ipv4/ip_gre.c:	   ICMP in the real Internet is absolutely infeasible.
net/ipv4/ip_gre.c:	 * ICMP in the real Internet is absolutely infeasible.


Normally, AFAICT, the FCS does not get passed to the operating system
since its a link layer mechanism.  If ERSPAN is passing the FCS when it
mirrors frames (does it mirror frames or packets, I don't know?) then
surely ERSPAN should provide a function to return the BSO value.

So IMHO this patch seems like a just pretense and not really doing
anything.

Hope this helps,
Tobin.

Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's len

[William Tu]

On Wed, May 16, 2018 at 3:24 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
> On Wed, May 16, 2018 at 07:05:34AM -0700, William Tu wrote:
>> On Mon, May 14, 2018 at 10:33 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
>> > On Mon, May 14, 2018 at 04:54:36PM -0700, William Tu wrote:
>> >> Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
>> >> handled.  BSO has 4 possible values:
>> >>   00 --> Good frame with no error, or unknown integrity
>> >>   11 --> Payload is a Bad Frame with CRC or Alignment Error
>> >>   01 --> Payload is a Short Frame
>> >>   10 --> Payload is an Oversized Frame
>> >>
>> >> Based the short/oversized definitions in RFC1757, the patch sets
>> >> the bso bit based on the mirrored packet's size.
>> >>
>> >> Reported-by: Xiaoyan Jin <xiaoyanj@vmware.com>
>> >> Signed-off-by: William Tu <u9012063@gmail.com>
>> >> ---
>> >>  include/net/erspan.h | 25 +++++++++++++++++++++++++
>> >>  1 file changed, 25 insertions(+)
>> >>
>> >> diff --git a/include/net/erspan.h b/include/net/erspan.h
>> >> index d044aa60cc76..5eb95f78ad45 100644
>> >> --- a/include/net/erspan.h
>> >> +++ b/include/net/erspan.h
>> >> @@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
>> >>       return htonl((u32)h_usecs);
>> >>  }
>> >>
>> >> +/* ERSPAN BSO (Bad/Short/Oversized)
>> >> + *   00b --> Good frame with no error, or unknown integrity
>> >> + *   01b --> Payload is a Short Frame
>> >> + *   10b --> Payload is an Oversized Frame
>> >> + *   11b --> Payload is a Bad Frame with CRC or Alignment Error
>> >> + */
>> >> +enum erspan_bso {
>> >> +     BSO_NOERROR,
>> >> +     BSO_SHORT,
>> >> +     BSO_OVERSIZED,
>> >> +     BSO_BAD,
>> >> +};
>> >
>> > If we are relying on the values perhaps this would be clearer
>> >
>> >         BSO_NOERROR     = 0x00,
>> >         BSO_SHORT       = 0x01,
>> >         BSO_OVERSIZED   = 0x02,
>> >         BSO_BAD         = 0x03,
>> >
>>
>> Yes, thanks. I will change in v2.
>>
>> >> +
>> >> +static inline u8 erspan_detect_bso(struct sk_buff *skb)
>> >> +{
>> >> +     if (skb->len < ETH_ZLEN)
>> >> +             return BSO_SHORT;
>> >> +
>> >> +     if (skb->len > ETH_FRAME_LEN)
>> >> +             return BSO_OVERSIZED;
>> >> +
>> >> +     return BSO_NOERROR;
>> >> +}
>> >
>> > Without having much contextual knowledge around this patch; should we be
>> > doing some check on CRC or alignment (at some stage)?  Having BSO_BAD
>> > seems to imply so?
>> >
>>
>> The definition of BSO_BAD:
>> etherStatsCRCAlignErrors OBJECT-TYPE
>>               SYNTAX Counter
>>               ACCESS read-only
>>               STATUS mandatory
>>               DESCRIPTION
>>                   "The total number of packets received that
>>                   had a length (excluding framing bits, but
>>                   including FCS octets) of between 64 and 1518
>>                   octets, inclusive, but but had either a bad
>>                   Frame Check Sequence (FCS) with an integral
>>                   number of octets (FCS Error) or a bad FCS with
>>                   a non-integral number of octets (Alignment Error)."
>>
>> But I don't know how to check CRC error at this code point.
>> Isn't it done by the NIC hardware?
>
> I'll just start with; I don't know anything about ERSPAN
>
>         "ERSPAN is a Cisco proprietary feature and is available only to
>         Catalyst 6500, 7600, Nexus, and ASR 1000 platforms to date. The
>         ASR 1000 supports ERSPAN source (monitoring) only on Fast
>         Ethernet, Gigabit Ethernet, and port-channel interfaces."
>
> https://supportforums.cisco.com/t5/network-infrastructure-documents/understanding-span-rspan-and-erspan/ta-p/3144951
>
> I dug around a bit and none of the files that currently import erspan.h
> actually use the 'bso' field
>
> $ grep bso $(git grep -l 'erspan\.h')
> include/net/erspan.h:   u8 bso = 0; /* Bad/Short/Oversized */
> include/net/erspan.h:   ershdr->en = bso;
> net/ipv4/ip_gre.c:         ICMP in the real Internet is absolutely infeasible.
> net/ipv4/ip_gre.c:       * ICMP in the real Internet is absolutely infeasible.
>
Yes, that's expected.

>
> Normally, AFAICT, the FCS does not get passed to the operating system
> since its a link layer mechanism.  If ERSPAN is passing the FCS when it
> mirrors frames (does it mirror frames or packets, I don't know?) then
> surely ERSPAN should provide a function to return the BSO value.

It mirrors layer 2 ethernet frame, so no FCS is passing.

>
> So IMHO this patch seems like a just pretense and not really doing
> anything.
>
The purpose is to set the BSO bit according to the spec, so that
ERSPAN monitor can interpret the mirrored traffic.

Thanks,
William