On 02-06-2020 16:29, Willem de Bruijn wrote:
> On Tue, Jun 2, 2020 at 4:05 AM Victor Julien <victor@xxxxxxxxxxxx> wrote:
>>
>> Introduce a new flag (TP_STATUS_CSUM_UNNECESSARY) to indicate
>> that the driver has completely validated the checksums in the packet.
>>
>> The TP_STATUS_CSUM_UNNECESSARY flag differs from TP_STATUS_CSUM_VALID
>> in that the new flag will only be set if all the layers are valid,
>> while TP_STATUS_CSUM_VALID is set as well if only the IP layer is valid.
>
> transport, not ip checksum.
Allow me a n00b question: what does transport refer to here? Things like
ethernet? It isn't clear to me from the doc.
(happy to follow up with a patch to clarify the doc when I understand
things better)
> But as I understand it drivers set CHECKSUM_COMPLETE if they fill in
> skb->csum over the full length of the packet. This does not
> necessarily imply that any of the checksum fields in the packet are
> valid yet (see also skb->csum_valid). Protocol code later compares
> checksum fields against this using __skb_checksum_validate_complete and friends.
>
> But packet sockets may be called before any of this, however. So I wonder
> how valid the checksum really is right now when setting TP_STATUS_CSUM_VALID.
> I assume it's correct, but don't fully understand where the validation
> has taken place..
I guess I'm more confused now about what TP_STATUS_CSUM_VALID actually
means. It sounds almost like the opposite of TP_STATUS_CSUMNOTREADY, but
I'm not sure I understand what the value would be.
It would be great if someone could help clear this up. Everything I
thought I knew/understood so far has been proven wrong, so I'm not too
confident about my patch anymore...
> Similar to commit 682f048bd494 ("af_packet: pass checksum validation
> status to the user"), please update tpacket_rcv and packet_rcv.
Ah yes, good catch. Will add it there as well.
> Note also that net-next is currently closed.
Should I hold off on sending a v3 until it reopens?
Regards / Groeten,
Victor
>
>
>
>> for convenience there are also the following defines::
>>
>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>> index 3d884d68eb30..76a5c762e2e0 100644
>> --- a/include/uapi/linux/if_packet.h
>> +++ b/include/uapi/linux/if_packet.h
>> @@ -113,6 +113,7 @@ struct tpacket_auxdata {
>> #define TP_STATUS_BLK_TMO (1 << 5)
>> #define TP_STATUS_VLAN_TPID_VALID (1 << 6) /* auxdata has valid tp_vlan_tpid */
>> #define TP_STATUS_CSUM_VALID (1 << 7)
>> +#define TP_STATUS_CSUM_UNNECESSARY (1 << 8)
>>
>> /* Tx ring - header status */
>> #define TP_STATUS_AVAILABLE 0
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 29bd405adbbd..94e213537646 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -2215,10 +2215,13 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>>
>> if (skb->ip_summed == CHECKSUM_PARTIAL)
>> status |= TP_STATUS_CSUMNOTREADY;
>> - else if (skb->pkt_type != PACKET_OUTGOING &&
>> - (skb->ip_summed == CHECKSUM_COMPLETE ||
>> - skb_csum_unnecessary(skb)))
>> - status |= TP_STATUS_CSUM_VALID;
>> + else if (skb->pkt_type != PACKET_OUTGOING) {
>> + if (skb->ip_summed == CHECKSUM_UNNECESSARY)
>> + status |= TP_STATUS_CSUM_UNNECESSARY | TP_STATUS_CSUM_VALID;
>> + else if (skb->ip_summed == CHECKSUM_COMPLETE ||
>> + skb_csum_unnecessary(skb))
>> + status |= TP_STATUS_CSUM_VALID;
>> + }
>>
>> if (snaplen > res)
>> snaplen = res;
>> --
>> 2.17.1
>>
--
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------
