Messages in this thread | | | From | Willem de Bruijn <> | Date | Fri, 20 Mar 2015 11:29:20 -0400 | Subject | Re: [PATCH 2/2] af_packet: pass checksum validation status to the user |
| |
On Fri, Mar 20, 2015 at 2:46 AM, Alexander Drozdov <al.drozdov@gmail.com> wrote: > > 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?
Yes. That's a very good example.
>> 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.
Thanks.
> >>> >>> >>>> 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 >>>>> >>> >>> >
| |