Patchwork bpf filter : support for vlan tag

login
register
mail settings
Submitter Eric Dumazet
Date Oct. 16, 2012, 11:28 a.m.
Message ID <1350386917.3954.890.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/191791/
State RFC
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Oct. 16, 2012, 11:28 a.m.
On Tue, 2012-10-16 at 13:00 +0200, Daniel Borkmann wrote:
> On Tue, Oct 16, 2012 at 8:46 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Mon, 2012-10-15 at 19:10 -0700, Ani Sinha wrote:
> >> Hi :
> >>
> >> I was looking at the kernel side implementation of the BPF filter. I
> >> do not see any code that supports filtering of packets based on
> >> provided vlan tag information from the skbuff. This will make it
> >> impossible to provide any filter to tcpdump that will filter packets
> >> based on the tag information if libpcap uses the kernel filter.
> >>
> >> Any help will be much appreciated.
> >
> > Right, we need a basic support, using a new ancillary definition.
> >
> > Is the following patch enough to address your need, or do you also need
> > access to vlan_tx_tag_present() ?
> 
> I like this patch, it's especially useful to speed up processing for
> packet analyzers. vlan_tx_tag_present() might also be good to have if
> this doesn't waste to much room for future ancillary operations.

There is plenty of room in ancillary space

Note that if speed is needed, we also want to update various JIT
implementations.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
danborkmann@iogearbox.net - Oct. 16, 2012, 11:34 a.m.
On Tue, Oct 16, 2012 at 1:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-10-16 at 13:00 +0200, Daniel Borkmann wrote:
>> On Tue, Oct 16, 2012 at 8:46 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Mon, 2012-10-15 at 19:10 -0700, Ani Sinha wrote:
>> >> Hi :
>> >>
>> >> I was looking at the kernel side implementation of the BPF filter. I
>> >> do not see any code that supports filtering of packets based on
>> >> provided vlan tag information from the skbuff. This will make it
>> >> impossible to provide any filter to tcpdump that will filter packets
>> >> based on the tag information if libpcap uses the kernel filter.
>> >>
>> >> Any help will be much appreciated.
>> >
>> > Right, we need a basic support, using a new ancillary definition.
>> >
>> > Is the following patch enough to address your need, or do you also need
>> > access to vlan_tx_tag_present() ?
>>
>> I like this patch, it's especially useful to speed up processing for
>> packet analyzers. vlan_tx_tag_present() might also be good to have if
>> this doesn't waste to much room for future ancillary operations.
>
> There is plenty of room in ancillary space
>
> Note that if speed is needed, we also want to update various JIT
> implementations.

Thanks for the patch. My kernel was still compiling and mutt was
already open for a submission on the vlan_tx_tag_present() operation.
:-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ani Sinha - Oct. 16, 2012, 4:54 p.m.
On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:

>
> Note that if speed is needed, we also want to update various JIT
> implementations.
>

Thanks Eric for the patch. I will take a look at it. We also need the
JIT module support as well (for x86 and x86_64) since we use the JIT
filter. I have been studying the various instructions and how other
filter operations have been implemented using them but it will take me
a little bit more time to understand the code.

Cheers,
ani
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 16, 2012, 5:06 p.m.
On Tue, 2012-10-16 at 09:54 -0700, Ani Sinha wrote:
> On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> >
> > Note that if speed is needed, we also want to update various JIT
> > implementations.
> >
> 
> Thanks Eric for the patch. I will take a look at it. We also need the
> JIT module support as well (for x86 and x86_64) since we use the JIT
> filter. I have been studying the various instructions and how other
> filter operations have been implemented using them but it will take me
> a little bit more time to understand the code.

Dont worry, I'll do the JIT part (but only for x86_64, we dont support
i386)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ani Sinha - Oct. 19, 2012, 6:32 p.m.
how about this?

On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> @@ -341,6 +342,12 @@ load_b:
>                 case BPF_S_ANC_CPU:
>                         A = raw_smp_processor_id();
>                         continue;
> +               case BPF_S_ANC_VLAN_TAG:
> +                       A = vlan_tx_tag_get(skb);
> +                       continue;
> +               case BPF_S_ANC_VLAN_TAG_PRESENT:
> +                       A = !!vlan_tx_tag_present(skb);
> +                       continue;
>                 case BPF_S_ANC_NLATTR: {
>                         struct nlattr *nla;


+               case BPF_S_ANC_VLAN_TAG:
+                       if (!vlan_tx_tag_present(skb)) {
+                               return 0;
+                       }
+                       A = vlan_tx_tag_get(skb);
+                       continue;
+               case BPF_S_ANC_VLAN_TAG_PRESENT :
+                       A = !! vlan_tx_tag_present(skb);
+                       continue;

Now, is there any particular reason we are not using clan_get_tag() api?

Thanks
ani
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
danborkmann@iogearbox.net - Oct. 19, 2012, 8:53 p.m.
On Fri, Oct 19, 2012 at 8:32 PM, Ani Sinha <ani@aristanetworks.com> wrote:
> how about this?
>
> On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> @@ -341,6 +342,12 @@ load_b:
>>                 case BPF_S_ANC_CPU:
>>                         A = raw_smp_processor_id();
>>                         continue;
>> +               case BPF_S_ANC_VLAN_TAG:
>> +                       A = vlan_tx_tag_get(skb);
>> +                       continue;
>> +               case BPF_S_ANC_VLAN_TAG_PRESENT:
>> +                       A = !!vlan_tx_tag_present(skb);
>> +                       continue;
>>                 case BPF_S_ANC_NLATTR: {
>>                         struct nlattr *nla;
>
>
> +               case BPF_S_ANC_VLAN_TAG:
> +                       if (!vlan_tx_tag_present(skb)) {
> +                               return 0;
> +                       }
> +                       A = vlan_tx_tag_get(skb);
> +                       continue;

I didn't look into the code, but I assume that if no vlan is present,
then vlan_tx_tag_get might return 0 anyway. Also, your return is
simply wrong, since then after this instruction you leave the *whole*
BPF machine ignoring the rest of the filter program to process ...

> +               case BPF_S_ANC_VLAN_TAG_PRESENT :
> +                       A = !! vlan_tx_tag_present(skb);
> +                       continue;
>
> Now, is there any particular reason we are not using clan_get_tag() api?
>
> Thanks
> ani
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ani Sinha - Oct. 19, 2012, 9:02 p.m.
On Fri, Oct 19, 2012 at 1:53 PM, Daniel Borkmann
<danborkmann@iogearbox.net> wrote:
> On Fri, Oct 19, 2012 at 8:32 PM, Ani Sinha <ani@aristanetworks.com> wrote:
>> how about this?
>>
>> On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>> @@ -341,6 +342,12 @@ load_b:
>>>                 case BPF_S_ANC_CPU:
>>>                         A = raw_smp_processor_id();
>>>                         continue;
>>> +               case BPF_S_ANC_VLAN_TAG:
>>> +                       A = vlan_tx_tag_get(skb);
>>> +                       continue;
>>> +               case BPF_S_ANC_VLAN_TAG_PRESENT:
>>> +                       A = !!vlan_tx_tag_present(skb);
>>> +                       continue;
>>>                 case BPF_S_ANC_NLATTR: {
>>>                         struct nlattr *nla;
>>
>>
>> +               case BPF_S_ANC_VLAN_TAG:
>> +                       if (!vlan_tx_tag_present(skb)) {
>> +                               return 0;
>> +                       }
>> +                       A = vlan_tx_tag_get(skb);
>> +                       continue;
>
> I didn't look into the code, but I assume that if no vlan is present,
> then vlan_tx_tag_get might return 0 anyway.

This might not be true all the time. So it's always safe to do this
check before returning the VLANID and throw some kind of error if the
vlan ID is not set.


 Also, your return is
> simply wrong, since then after this instruction you leave the *whole*
> BPF machine ignoring the rest of the filter program to process ...


I had done that because I can see in other parts of that state machine
that in error condition the code simply stops processing the packet. I
am not sure how else to handle the error case.

Ani
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
danborkmann@iogearbox.net - Oct. 20, 2012, 11:42 a.m.
On Fri, Oct 19, 2012 at 11:02 PM, Ani Sinha <ani@aristanetworks.com> wrote:
> On Fri, Oct 19, 2012 at 1:53 PM, Daniel Borkmann
> <danborkmann@iogearbox.net> wrote:
>> On Fri, Oct 19, 2012 at 8:32 PM, Ani Sinha <ani@aristanetworks.com> wrote:
>>> how about this?
>>>
>>> On Tue, Oct 16, 2012 at 4:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>> @@ -341,6 +342,12 @@ load_b:
>>>>                 case BPF_S_ANC_CPU:
>>>>                         A = raw_smp_processor_id();
>>>>                         continue;
>>>> +               case BPF_S_ANC_VLAN_TAG:
>>>> +                       A = vlan_tx_tag_get(skb);
>>>> +                       continue;
>>>> +               case BPF_S_ANC_VLAN_TAG_PRESENT:
>>>> +                       A = !!vlan_tx_tag_present(skb);
>>>> +                       continue;
>>>>                 case BPF_S_ANC_NLATTR: {
>>>>                         struct nlattr *nla;
>>>
>>>
>>> +               case BPF_S_ANC_VLAN_TAG:
>>> +                       if (!vlan_tx_tag_present(skb)) {
>>> +                               return 0;
>>> +                       }
>>> +                       A = vlan_tx_tag_get(skb);
>>> +                       continue;
>>
>> I didn't look into the code, but I assume that if no vlan is present,
>> then vlan_tx_tag_get might return 0 anyway.
>
> This might not be true all the time. So it's always safe to do this
> check before returning the VLANID and throw some kind of error if the
> vlan ID is not set.

But wasn't this the reason why Eric added BPF_S_ANC_VLAN_TAG_PRESENT ?
Or is your objection performance related?

>  Also, your return is
>> simply wrong, since then after this instruction you leave the *whole*
>> BPF machine ignoring the rest of the filter program to process ...
>
> I had done that because I can see in other parts of that state machine
> that in error condition the code simply stops processing the packet. I
> am not sure how else to handle the error case.

See comment above. In my opinion the other cases are more severe like
divide by zero and the like, but okay ..
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
danborkmann@iogearbox.net - Oct. 26, 2012, 8:05 a.m.
On Tue, Oct 16, 2012 at 1:28 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2012-10-16 at 13:00 +0200, Daniel Borkmann wrote:
>> On Tue, Oct 16, 2012 at 8:46 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Mon, 2012-10-15 at 19:10 -0700, Ani Sinha wrote:
>> >> I was looking at the kernel side implementation of the BPF filter. I
>> >> do not see any code that supports filtering of packets based on
>> >> provided vlan tag information from the skbuff. This will make it
>> >> impossible to provide any filter to tcpdump that will filter packets
>> >> based on the tag information if libpcap uses the kernel filter.
>> >>
>> >> Any help will be much appreciated.
>> >
>> > Right, we need a basic support, using a new ancillary definition.
>> >
>> > Is the following patch enough to address your need, or do you also need
>> > access to vlan_tx_tag_present() ?
>>
>> I like this patch, it's especially useful to speed up processing for
>> packet analyzers. vlan_tx_tag_present() might also be good to have if
>> this doesn't waste to much room for future ancillary operations.
>
> There is plenty of room in ancillary space
>
> Note that if speed is needed, we also want to update various JIT
> implementations.

Eric, can you submit this patch to net-next if there are no objections
from your side regarding the follow-up comments? Big thanks.

> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 24d251f..c9f0005 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -123,6 +123,8 @@ enum {
>         BPF_S_ANC_CPU,
>         BPF_S_ANC_ALU_XOR_X,
>         BPF_S_ANC_SECCOMP_LD_W,
> +       BPF_S_ANC_VLAN_TAG,
> +       BPF_S_ANC_VLAN_TAG_PRESENT,
>  };
>
>  #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/uapi/linux/filter.h b/include/uapi/linux/filter.h
> index 3d79224..9cfde69 100644
> --- a/include/uapi/linux/filter.h
> +++ b/include/uapi/linux/filter.h
> @@ -127,7 +127,9 @@ struct sock_fprog { /* Required for SO_ATTACH_FILTER. */
>  #define SKF_AD_RXHASH  32
>  #define SKF_AD_CPU     36
>  #define SKF_AD_ALU_XOR_X       40
> -#define SKF_AD_MAX     44
> +#define SKF_AD_VLAN_TAG        44
> +#define SKF_AD_VLAN_TAG_PRESENT 48
> +#define SKF_AD_MAX     52
>  #define SKF_NET_OFF   (-0x100000)
>  #define SKF_LL_OFF    (-0x200000)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3d92ebb..5a114d4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -39,6 +39,7 @@
>  #include <linux/reciprocal_div.h>
>  #include <linux/ratelimit.h>
>  #include <linux/seccomp.h>
> +#include <linux/if_vlan.h>
>
>  /* No hurry in this branch
>   *
> @@ -341,6 +342,12 @@ load_b:
>                 case BPF_S_ANC_CPU:
>                         A = raw_smp_processor_id();
>                         continue;
> +               case BPF_S_ANC_VLAN_TAG:
> +                       A = vlan_tx_tag_get(skb);
> +                       continue;
> +               case BPF_S_ANC_VLAN_TAG_PRESENT:
> +                       A = !!vlan_tx_tag_present(skb);
> +                       continue;
>                 case BPF_S_ANC_NLATTR: {
>                         struct nlattr *nla;
>
> @@ -600,6 +607,8 @@ int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
>                         ANCILLARY(RXHASH);
>                         ANCILLARY(CPU);
>                         ANCILLARY(ALU_XOR_X);
> +                       ANCILLARY(VLAN_TAG);
> +                       ANCILLARY(VLAN_TAG_PRESENT);
>                         }
>                 }
>                 ftest->code = code;
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 26, 2012, 8:16 a.m.
On Fri, 2012-10-26 at 10:05 +0200, Daniel Borkmann wrote:

> Eric, can you submit this patch to net-next if there are no objections
> from your side regarding the follow-up comments? Big thanks.

Yep, I am preparing two patches :

1) Generic support

2) x86_64 jit support



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 24d251f..c9f0005 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -123,6 +123,8 @@  enum {
 	BPF_S_ANC_CPU,
 	BPF_S_ANC_ALU_XOR_X,
 	BPF_S_ANC_SECCOMP_LD_W,
+	BPF_S_ANC_VLAN_TAG,
+	BPF_S_ANC_VLAN_TAG_PRESENT,
 };
 
 #endif /* __LINUX_FILTER_H__ */
diff --git a/include/uapi/linux/filter.h b/include/uapi/linux/filter.h
index 3d79224..9cfde69 100644
--- a/include/uapi/linux/filter.h
+++ b/include/uapi/linux/filter.h
@@ -127,7 +127,9 @@  struct sock_fprog {	/* Required for SO_ATTACH_FILTER. */
 #define SKF_AD_RXHASH	32
 #define SKF_AD_CPU	36
 #define SKF_AD_ALU_XOR_X	40
-#define SKF_AD_MAX	44
+#define SKF_AD_VLAN_TAG	44
+#define SKF_AD_VLAN_TAG_PRESENT 48
+#define SKF_AD_MAX	52
 #define SKF_NET_OFF   (-0x100000)
 #define SKF_LL_OFF    (-0x200000)
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 3d92ebb..5a114d4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -39,6 +39,7 @@ 
 #include <linux/reciprocal_div.h>
 #include <linux/ratelimit.h>
 #include <linux/seccomp.h>
+#include <linux/if_vlan.h>
 
 /* No hurry in this branch
  *
@@ -341,6 +342,12 @@  load_b:
 		case BPF_S_ANC_CPU:
 			A = raw_smp_processor_id();
 			continue;
+		case BPF_S_ANC_VLAN_TAG:
+			A = vlan_tx_tag_get(skb);
+			continue;
+		case BPF_S_ANC_VLAN_TAG_PRESENT:
+			A = !!vlan_tx_tag_present(skb);
+			continue;
 		case BPF_S_ANC_NLATTR: {
 			struct nlattr *nla;
 
@@ -600,6 +607,8 @@  int sk_chk_filter(struct sock_filter *filter, unsigned int flen)
 			ANCILLARY(RXHASH);
 			ANCILLARY(CPU);
 			ANCILLARY(ALU_XOR_X);
+			ANCILLARY(VLAN_TAG);
+			ANCILLARY(VLAN_TAG_PRESENT);
 			}
 		}
 		ftest->code = code;