On Tue, Jul 28, 2020 at 11:06:08PM +0200, Petr Machata wrote:
>
> Kurt Kanzenbach <k...@linutronix.de> writes:
>
> > On Mon Jul 27 2020, Petr Machata wrote:
> >> So this looks good, and works, but I'm wondering about one thing.
> >
> > Thanks for testing.
> >
> >>
> >> Your code (and evidently most drivers as well) use a different check
> >> than mlxsw, namely skb->len + ETH_HLEN < X. When I print_hex_dump()
> >> skb_mac_header(skb), skb->len in mlxsw with some test packet, I get e.g.
> >> this:
> >>
> >> 00000000259a4db7: 01 00 5e 00 01 81 00 02 c9 a4 e4 e1 08 00 45 00
> >> ..^...........E.
> >> 000000005f29f0eb: 00 48 0d c9 40 00 01 11 c8 59 c0 00 02 01 e0 00
> >> .H..@....Y......
> >> 00000000f3663e9e: 01 81 01 3f 01 3f 00 34 9f d3 00 02 00 2c 00 00
> >> ...?.?.4.....,..
> >> ^sp^^ ^dp^^ ^len^ ^cks^ ^len^
> >> 00000000b3914606: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02
> >> ................
> >> 000000002e7828ea: c9 ff fe a4 e4 e1 00 01 09 fa 00 00 00 00 00 00
> >> ................
> >> 000000000b98156e: 00 00 00 00 00 00
> >> ......
> >>
> >> Both UDP and PTP length fields indicate that the payload ends exactly at
> >> the end of the dump. So apparently skb->len contains all the payload
> >> bytes, including the Ethernet header.
> >>
> >> Is that the case for other drivers as well? Maybe mlxsw is just missing
> >> some SKB magic in the driver.
> >
> > So I run some tests (on other hardware/drivers) and it seems like that
> > the skb->len usually doesn't include the ETH_HLEN. Therefore, it is
> > added to the check.
> >
> > Looking at the driver code:
> >
> > |static void mlxsw_sp_rx_sample_listener(struct sk_buff *skb, u8 local_port,
> > | void *trap_ctx)
> > |{
> > | [...]
> > | /* The sample handler expects skb->data to point to the start of the
> > | * Ethernet header.
> > | */
> > | skb_push(skb, ETH_HLEN);
> > | mlxsw_sp_sample_receive(mlxsw_sp, skb, local_port);
> > |}
> >
> > Maybe that's the issue here?
>
> Correct, mlxsw pushes the header very soon. Given that both
> ptp_classify_raw() and eth_type_trans() that are invoked later assume
> the header, it is reasonable. I have shuffled the pushes around and have
> a patch that both works and I think is correct.
Would it make more sense to do:
u8 *data = skb_mac_header(skb);
u8 *ptr = data;
if (type & PTP_CLASS_VLAN)
ptr += VLAN_HLEN;
switch (type & PTP_CLASS_PMASK) {
case PTP_CLASS_IPV4:
ptr += IPV4_HLEN(ptr) + UDP_HLEN;
break;
case PTP_CLASS_IPV6:
ptr += IP6_HLEN + UDP_HLEN;
break;
case PTP_CLASS_L2:
break;
default:
return NULL;
}
ptr += ETH_HLEN;
if (ptr + 34 > skb->data + skb->len)
return NULL;
return ptr;
in other words, compare pointers, so that whether skb_push() etc has
been used on the skb is irrelevant?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
