| Message ID | 1526342076-43714-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] erspan: set bso bit based on mirrored packet's len | expand |
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.
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
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.
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
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);
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(+)