[PATCH 1/2] af_packet: make tpacket_rcv to not set status value before run_filter

[PATCH 1/2] af_packet: make tpacket_rcv to not set status value before run_filter

[Alexander Drozdov]

It is just an optimization. We don't need the value of status variable
if the packet is filtered.

Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com>
---
 net/packet/af_packet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f8db706..6ecf8dd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1910,14 +1910,15 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	if (skb->ip_summed == CHECKSUM_PARTIAL)
-		status |= TP_STATUS_CSUMNOTREADY;
-
 	snaplen = skb->len;
 
 	res = run_filter(skb, sk, snaplen);
 	if (!res)
 		goto drop_n_restore;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL)
+		status |= TP_STATUS_CSUMNOTREADY;
+
 	if (snaplen > res)
 		snaplen = res;
 
-- 
1.9.1

[PATCH 2/2] af_packet: pass checksum validation status to the user

[Alexander Drozdov]

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.

For now, the flag may be set for incoming packets only.

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;
+
 		aux.tp_len = PACKET_SKB_CB(skb)->origlen;
 		aux.tp_snaplen = skb->len;
 		aux.tp_mac = 0;
-- 
1.9.1

Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Willem de Bruijn]

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? You cannot change the semantics of the flag afterwards.
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
>

Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Alexander Drozdov]

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, but that is not true
for me (in my tests, skb->ip_summed ==
CHECKSUM_UNNECESSARY for forwarded packets in some
cases).

> 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.

> 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
>>

Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Willem de Bruijn]

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?

>> 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 <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
>>>
>
>

Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Alexander Drozdov]

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
>>>>
>>
>>

Re: [PATCH 2/2] af_packet: pass checksum validation status to the user

[Willem de Bruijn]

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
>>>>>
>>>
>>>
>

[PATCH V2 1/2] af_packet: make tpacket_rcv to not set status value before run_filter

[Alexander Drozdov]

It is just an optimization. We don't need the value of status variable
if the packet is filtered.

Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com>
---
 net/packet/af_packet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f8db706..6ecf8dd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1910,14 +1910,15 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	if (skb->ip_summed == CHECKSUM_PARTIAL)
-		status |= TP_STATUS_CSUMNOTREADY;
-
 	snaplen = skb->len;
 
 	res = run_filter(skb, sk, snaplen);
 	if (!res)
 		goto drop_n_restore;
+
+	if (skb->ip_summed == CHECKSUM_PARTIAL)
+		status |= TP_STATUS_CSUMNOTREADY;
+
 	if (snaplen > res)
 		snaplen = res;
 
-- 
1.9.1

[PATCH V2 2/2] af_packet: pass checksum validation status to the user

[Alexander Drozdov]

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.

For now, the flag may be set for incoming packets only.

Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com>
Cc: Willem de Bruijn <willemb@google.com>
---
 Documentation/networking/packet_mmap.txt | 13 ++++++++++---
 include/uapi/linux/if_packet.h           |  1 +
 net/packet/af_packet.c                   |  9 +++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/packet_mmap.txt b/Documentation/networking/packet_mmap.txt
index a6d7cb9..daa015a 100644
--- a/Documentation/networking/packet_mmap.txt
+++ b/Documentation/networking/packet_mmap.txt
@@ -440,9 +440,10 @@ and the following flags apply:
 +++ Capture process:
      from include/linux/if_packet.h
 
-     #define TP_STATUS_COPY          2 
-     #define TP_STATUS_LOSING        4 
-     #define TP_STATUS_CSUMNOTREADY  8 
+     #define TP_STATUS_COPY          (1 << 1)
+     #define TP_STATUS_LOSING        (1 << 2)
+     #define TP_STATUS_CSUMNOTREADY  (1 << 3)
+     #define TP_STATUS_CSUM_VALID    (1 << 7)
 
 TP_STATUS_COPY        : This flag indicates that the frame (and associated
                         meta information) has been truncated because it's 
@@ -466,6 +467,12 @@ TP_STATUS_CSUMNOTREADY: currently it's used for outgoing IP packets which
                         reading the packet we should not try to check the 
                         checksum. 
 
+TP_STATUS_CSUM_VALID  : This flag indicates that at least the transport
+                        header checksum of the packet has been already
+                        validated on the kernel side. If the flag is not set
+                        then we are free to check the checksum by ourselves
+                        provided that TP_STATUS_CSUMNOTREADY is also not set.
+
 for convenience there are also the following defines:
 
      #define TP_STATUS_KERNEL        0
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;
+
 		aux.tp_len = PACKET_SKB_CB(skb)->origlen;
 		aux.tp_snaplen = skb->len;
 		aux.tp_mac = 0;
-- 
1.9.1

Re: [PATCH V2 2/2] af_packet: pass checksum validation status to the user

[David Miller]

From: Alexander Drozdov <al.drozdov@gmail.com>
Date: Mon, 23 Mar 2015 09:11:13 +0300

> 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.
> 
> For now, the flag may be set for incoming packets only.
> 
> Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com>

Applied to net-next

Re: [PATCH V2 1/2] af_packet: make tpacket_rcv to not set status value before run_filter

[David Miller]

From: Alexander Drozdov <al.drozdov@gmail.com>
Date: Mon, 23 Mar 2015 09:11:12 +0300

> It is just an optimization. We don't need the value of status variable
> if the packet is filtered.
> 
> Signed-off-by: Alexander Drozdov <al.drozdov@gmail.com>

Applied to net-next