On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov <[email protected]> wrote: > On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn <[email protected]> > wrote: >> >> On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov <[email protected]> >> 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? >> 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. Please also note the flag and semantics in Documentation/networking/packet_mmap.txt > > >> Better to support both directions from the start. >> >>> Signed-off-by: Alexander Drozdov <[email protected]> >>> --- >>> 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 >>> > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
