Hi Herbert,
I am looking at "net: Fix skb csum races when peeking". The context is
that I'm seeing checksum faults after backporting mlx5 from driver.
The handling of the checksum faults in interesting in itself. One side
effect is that we end up recalculating and doing the dump twice when
GRO is enabled. Another is that we validate checksum complete in a
shared skbuf with a knowingly bad skb->csum value.
In the checksum fault case we recalculate the checksum and find it was
actually correct in the packet. If the skb is not shared then we set
the skb->csum to the correct value and also set skb->csum_complete_sw
and skb->csum_valid. But if skb is shared we don't do any of that.
This is kind of odd since __skb_checksum_complete_head then returns
zero for an skb with checksum_complete where the checksum complete
value is not correct. I'm not sure what the consequences of doing that
are (I don't think it's related to issues I'm seeing) but this doesn't
seem like a good thing. Maybe we should set skb->csum in this case
since we know it's already wrong and the fact that skbuf is shared
can't make it more wrong...
Tom
On Thu, 2017-01-19 at 17:29 -0800, Tom Herbert wrote:
Hi Herbert,
I am looking at "net: Fix skb csum races when peeking". The context is
that I'm seeing checksum faults after backporting mlx5 from driver.
The handling of the checksum faults in interesting in itself. One side
effect is that we end up recalculating and doing the dump twice when
GRO is enabled. Another is that we validate checksum complete in a
shared skbuf with a knowingly bad skb->csum value.
In the checksum fault case we recalculate the checksum and find it was
actually correct in the packet. If the skb is not shared then we set
the skb->csum to the correct value and also set skb->csum_complete_sw
and skb->csum_valid. But if skb is shared we don't do any of that.
This is kind of odd since __skb_checksum_complete_head then returns
zero for an skb with checksum_complete where the checksum complete
value is not correct. I'm not sure what the consequences of doing that
are (I don't think it's related to issues I'm seeing) but this doesn't
seem like a good thing. Maybe we should set skb->csum in this case
since we know it's already wrong and the fact that skbuf is shared
can't make it more wrong...
Why GRO can not change skb fields ?
skb can not be shared at GRO layer, so really this should be done there
if you want to avoid a double checksumming later.
You added all these skb fields, you know better than us ;)
On Thu, Jan 19, 2017 at 6:00 PM, Eric Dumazet [off-list ref] wrote:
On Thu, 2017-01-19 at 17:29 -0800, Tom Herbert wrote:
quoted
Hi Herbert,
I am looking at "net: Fix skb csum races when peeking". The context is
that I'm seeing checksum faults after backporting mlx5 from driver.
The handling of the checksum faults in interesting in itself. One side
effect is that we end up recalculating and doing the dump twice when
GRO is enabled. Another is that we validate checksum complete in a
shared skbuf with a knowingly bad skb->csum value.
In the checksum fault case we recalculate the checksum and find it was
actually correct in the packet. If the skb is not shared then we set
the skb->csum to the correct value and also set skb->csum_complete_sw
and skb->csum_valid. But if skb is shared we don't do any of that.
This is kind of odd since __skb_checksum_complete_head then returns
zero for an skb with checksum_complete where the checksum complete
value is not correct. I'm not sure what the consequences of doing that
are (I don't think it's related to issues I'm seeing) but this doesn't
seem like a good thing. Maybe we should set skb->csum in this case
since we know it's already wrong and the fact that skbuf is shared
can't make it more wrong...
Why GRO can not change skb fields ?
Sure, I just noted this fact. We don't typically expect the device to
be giving us miscomputed checksum complete values ;-)
skb can not be shared at GRO layer, so really this should be done there
if you want to avoid a double checksumming later.
You added all these skb fields, you know better than us ;)