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.
However, I find it odd that ptp_classify_raw() assumes that ->data
points at Ethernet, while ptp_parse_header() makes the contrary
assumption that ->len does not cover Ethernet. Those functions are
likely to be used just a couple calls away from each other, if not
outright next to each other.
I suspect that ti/cpts.c and ti/am65-cpts.c (patches 5 and 6) actually
hit an issue in this. ptp_classify_raw() is called without a surrounding
_push / _pull (unlike DSA), which would imply skb->data points at
Ethernet header, and indeed, the way the "data" variable is used
confirms it. (At the same time the code adds ETH_HLEN to skb->len, but
maybe it is just a cut'n'paste.) But then ptp_parse_header() is called,
and that makes the assumption that skb->len does not cover the Ethernet
header.
> I was also wondering about something else in that driver driver: The
> parsing code allows for ptp v1, but the message type was always fetched
> from offset 0 in the header. Is that indented?
Yeah, I noticed that as well. That was a bug in the mlxsw code. Good
riddance :)
