On Wed, May 16, 2018 at 3:24 PM, Tobin C. Harding <to...@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 <to...@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 <xiaoy...@vmware.com> >> >> Signed-off-by: William Tu <u9012...@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