On 29/07/2020 00:06, 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.
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.
Both drivers, in all cases, will get
->data points at Ethernet header and
->len covers ETH_HLEN
So, yes below check is incorrect, in general, and will be false always if other
calculation are correct
(only IPV4_HLEN(data + offset) can cause issues).
if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
return 0;
it might be good to update ptp_parse_header() documentation with expected state
of skb.
(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 :)
--
Best regards,
grygorii
