* Setting skb->csum in shared skb with csum-complete
@ 2017-01-20 1:29 Tom Herbert
2017-01-20 2:00 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Tom Herbert @ 2017-01-20 1:29 UTC (permalink / raw)
To: Herbert Xu, Linux Kernel Network Developers
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Setting skb->csum in shared skb with csum-complete
2017-01-20 1:29 Setting skb->csum in shared skb with csum-complete Tom Herbert
@ 2017-01-20 2:00 ` Eric Dumazet
2017-01-20 3:50 ` Tom Herbert
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2017-01-20 2:00 UTC (permalink / raw)
To: Tom Herbert; +Cc: Herbert Xu, Linux Kernel Network Developers
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 ;)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Setting skb->csum in shared skb with csum-complete
2017-01-20 2:00 ` Eric Dumazet
@ 2017-01-20 3:50 ` Tom Herbert
0 siblings, 0 replies; 3+ messages in thread
From: Tom Herbert @ 2017-01-20 3:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Herbert Xu, Linux Kernel Network Developers
On Thu, Jan 19, 2017 at 6:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 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 ?
>
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 ;)
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-01-20 3:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-20 1:29 Setting skb->csum in shared skb with csum-complete Tom Herbert
2017-01-20 2:00 ` Eric Dumazet
2017-01-20 3:50 ` Tom Herbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.