Messages in this thread | | | Date | Fri, 20 Mar 2015 09:46:07 +0300 | From | Alexander Drozdov <> | Subject | Re: [PATCH 2/2] af_packet: pass checksum validation status to the user |
| |
On 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote: > On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov <al.drozdov@gmail.com> wrote: >> On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn <willemb@google.com> >> wrote: >>> >>> On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov <al.drozdov@gmail.com> >>> wrote: >>>> >>>> Introduce TP_STATUS_CSUM_VALID tp_status flag to tell the >>>> af_packet user that at least the transport header checksum >>>> has been already validated. >>> >>> This changes the interface slightly. Processes should be treating >>> this previously unused bit as reserved and other flags have >>> been added to the bitmap in this manner as well, so this should >>> then be safe here, too. >>> >>>> For now, the flag may be set for incoming packets only. >>> >>> Why? >> >> I can't figure out how af_packet could get that the outgoing >> packet's checksum has been validated. "Checksumming on >> output" from skbuff.h tells that skb->ip_summed should >> equal to CHECKSUM_NONE in that case, > > CHECKSUM_UNNECESSARY is a valid flag on the outgoing > path according to that documentation. And, indeed, I also see > no checksum scrubbing on forwarding paths in practice (but I > may be wrong there, only took a quick glance). > >> but that is not true for me (in my tests, skb->ip_summed == >> CHECKSUM_UNNECESSARY for forwarded packets in some >> cases). > > When you disable hardware checksum offload and generate > packets locally, you do see the expected CHECKSUM_NONE > value?
Yes, I do, but see below.
>>> You cannot change the semantics of the flag afterwards. >> >> >> I think the semantics of the flag won't be changed if one set the flag >> for outgoing packets. If the flag is not set (for any directions) >> then that is not mean that the packet checksum is invalid. The user just can >> then >> checksum the packet by itself. So, the user may check the flag for any >> packet >> right now. > > I see. So it is a hint. Okay. It would be nice if it behaves as > expected in as many cases as possible from the outset. This > would include PACKET_OUTGOING and CHECKSUM_NONE.
I've just done some testing, and I've found that packets generated by 'nping --badsum' (socket(PF_INET, SOCK_RAW, IPPROTO_RAW)) have CHECKSUM_NONE when they are viewed by af_packet. I've used rather old Linux kernel for the tests, but isn't that a reason to not set TP_STATUS_CSUM_VALID for outgoing packets right now?
> Please also note the flag and semantics in > Documentation/networking/packet_mmap.txt
I'll do it and I'll resend the patches with the note.
>> >> >>> Better to support both directions from the start. >>> >>>> Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com> >>>> --- >>>> include/uapi/linux/if_packet.h | 1 + >>>> net/packet/af_packet.c | 9 +++++++++ >>>> 2 files changed, 10 insertions(+) >>>> >>>> diff --git a/include/uapi/linux/if_packet.h >>>> b/include/uapi/linux/if_packet.h >>>> index da2d668..053bd10 100644 >>>> --- a/include/uapi/linux/if_packet.h >>>> +++ b/include/uapi/linux/if_packet.h >>>> @@ -99,6 +99,7 @@ struct tpacket_auxdata { >>>> #define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid >>>> tp_vlan_tci */ >>>> #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) >>>> >>>> /* Tx ring - header status */ >>>> #define TP_STATUS_AVAILABLE 0 >>>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >>>> index 6ecf8dd..3f09dda 100644 >>>> --- a/net/packet/af_packet.c >>>> +++ b/net/packet/af_packet.c >>>> @@ -1918,6 +1918,10 @@ 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; >>>> >>>> if (snaplen > res) >>>> snaplen = res; >>>> @@ -3015,6 +3019,11 @@ static int packet_recvmsg(struct kiocb *iocb, >>>> struct socket *sock, >>>> aux.tp_status = TP_STATUS_USER; >>>> if (skb->ip_summed == CHECKSUM_PARTIAL) >>>> aux.tp_status |= TP_STATUS_CSUMNOTREADY; >>>> + else if (skb->pkt_type != PACKET_OUTGOING && >>>> + (skb->ip_summed == CHECKSUM_COMPLETE || >>>> + skb_csum_unnecessary(skb))) >>>> + aux.tp_status |= TP_STATUS_CSUM_VALID; >>>> + >>> >>> These two sections are near duplicates. I'd move the entire status >>> initialization, including existing TP_STATUS_USER and >>> TP_STATUS_CSUMNOTREADY fields to a helper function. >>> >>> It's a bit unfortunately that we have to use an extra bit to add a signal >>> that is a near inverse of the existing TP_STATUS_CSUMNOTREADY >>> (bar CHECKSUM_NONE). I do not immediately see a better way, >>> either, though. And tp_status has plenty room at 32 bits. >>> >>>> aux.tp_len = PACKET_SKB_CB(skb)->origlen; >>>> aux.tp_snaplen = skb->len; >>>> aux.tp_mac = 0; >>>> -- >>>> 1.9.1 >>>> >> >>
| |