From: Alexander Drozdov <hidden> Date: 2015-03-19 08:02:21
It is just an optimization. We don't need the value of status variable
if the packet is filtered.
Signed-off-by: Alexander Drozdov <redacted>
---
net/packet/af_packet.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Alexander Drozdov <hidden> Date: 2015-03-19 08:02:36
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 <redacted>
---
include/uapi/linux/if_packet.h | 1 +
net/packet/af_packet.c | 9 +++++++++
2 files changed, 10 insertions(+)
From: Willem de Bruijn <willemb@google.com> Date: 2015-03-19 15:30:20
On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov [off-list ref] 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.
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.
From: Alexander Drozdov <hidden> Date: 2015-03-19 18:08:54
On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn [off-list ref] wrote:
On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov [off-list ref] wrote:
quoted
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.
quoted
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.
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.
From: Willem de Bruijn <willemb@google.com> Date: 2015-03-19 18:50:37
On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov [off-list ref] wrote:
On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn [off-list ref]
wrote:
quoted
On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov [off-list ref]
wrote:
quoted
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.
quoted
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?
quoted
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
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.
From: Alexander Drozdov <hidden> Date: 2015-03-20 06:46:30
On 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote:
On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov [off-list ref] wrote:
quoted
On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn [off-list ref]
wrote:
quoted
On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov [off-list ref]
wrote:
quoted
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.
quoted
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).
quoted
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.
quoted
quoted
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.
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.
From: Willem de Bruijn <willemb@google.com> Date: 2015-03-20 15:29:59
On Fri, Mar 20, 2015 at 2:46 AM, Alexander Drozdov [off-list ref] wrote:
On 19.03.2015 21:50:03 +0300 Willem de Bruijn wrote:
quoted
On Thu, Mar 19, 2015 at 2:08 PM, Alexander Drozdov [off-list ref]
wrote:
quoted
On Thu, 19 Mar 2015 11:29:32 -0400, Willem de Bruijn [off-list ref]
wrote:
quoted
On Thu, Mar 19, 2015 at 4:01 AM, Alexander Drozdov
[off-list ref]
wrote:
quoted
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.
quoted
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).
quoted
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.
quoted
quoted
quoted
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.
quoted
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.
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.
From: Alexander Drozdov <hidden> Date: 2015-03-23 06:11:35
It is just an optimization. We don't need the value of status variable
if the packet is filtered.
Signed-off-by: Alexander Drozdov <redacted>
---
net/packet/af_packet.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Alexander Drozdov <hidden> Date: 2015-03-23 06:11:45
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 <redacted>
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(-)
@@ -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
From: David Miller <davem@davemloft.net> Date: 2015-03-24 02:01:50
From: Alexander Drozdov <redacted>
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 <redacted>