Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Pablo Neira Ayuso]

On Fri, Jul 25, 2014 at 01:25:35PM +0200, Daniel Borkmann wrote:
> [ also Cc'ing Willem, Pablo ]
> 
> On 07/25/2014 10:04 AM, Alexei Starovoitov wrote:
> >'sk_filter' name is used as 'struct sk_filter', function sk_filter() and
> >as variable 'sk_filter', which makes code hard to read.
> >Also it's easily confused with 'struct sock_filter'
> >Rename 'struct sk_filter' to 'struct bpf_prog' to clarify semantics and
> >align the name with generic BPF use model.
> 
> Agreed, as we went for kernel/bpf/, renaming makes absolutely sense.

My nft socket filtering changes are accomodated into struct sk_filter,
and will still be, so I still need some generic name there...

Please, leave this as it is.

> >The only ugly place is uapi/linux/netfilter/xt_bpf.h which
> >managed to expose kernel internal structure into uapi header.
> >Though it shouldn't even compile in user space, preserve the mess by
> >adding empty 'struct sk_filter;' there and type cast it to 'struct bpf_prog'
> >inside kernel in net/netfilter/xt_bpf.c
> >
> >Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> >---
> >
> >alternative fix for xt_bpf.h could be to replace:
> >         /* only used in the kernel */
> >	struct sk_filter *filter __attribute__((aligned(8)));
> >with
> >         /* only used in the kernel */
> >	void *filter __attribute__((aligned(8)));
> >
> >but this 'void *' approach may further break broken userspace,
> >whereas the fix implemented here is more seamless.
> 
> Yep, that's not good, 'struct sk_filter' should never have been in a uapi
> file actually.

You can just send me a patch to change it to void. It's an internal
kernel pointer as the comment states. There is **no** way that
userspace can lurk with that from iptables at all.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Daniel Borkmann]

On 07/25/2014 01:54 PM, Pablo Neira Ayuso wrote:
> On Fri, Jul 25, 2014 at 01:25:35PM +0200, Daniel Borkmann wrote:
>> [ also Cc'ing Willem, Pablo ]
>>
>> On 07/25/2014 10:04 AM, Alexei Starovoitov wrote:
>>> 'sk_filter' name is used as 'struct sk_filter', function sk_filter() and
>>> as variable 'sk_filter', which makes code hard to read.
>>> Also it's easily confused with 'struct sock_filter'
>>> Rename 'struct sk_filter' to 'struct bpf_prog' to clarify semantics and
>>> align the name with generic BPF use model.
>>
>> Agreed, as we went for kernel/bpf/, renaming makes absolutely sense.
>
> My nft socket filtering changes are accomodated into struct sk_filter,
> and will still be, so I still need some generic name there...

All the parts from filter.c which is BPF's core engine have been moved
into kernel/bpf/ to get it ready for tracing et al, since there is not
always a socket context anymore. The *whole* infrastructure around struct
sk_filter is [e]BPF and used in non-net related contexts as well, whereas
nft socket filtering is *only* for sockets. Due to the socket-only specific
use case why doesn't it make more sense to have a union in struct sock
around sk_filter (or however we name it) and only allow one of the two
being loaded on a socket?

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Willem de Bruijn]

>> >alternative fix for xt_bpf.h could be to replace:
>> >         /* only used in the kernel */
>> >     struct sk_filter *filter __attribute__((aligned(8)));
>> >with
>> >         /* only used in the kernel */
>> >     void *filter __attribute__((aligned(8)));
>> >
>> >but this 'void *' approach may further break broken userspace,
>> >whereas the fix implemented here is more seamless.
>>
>> Yep, that's not good, 'struct sk_filter' should never have been in a uapi
>> file actually.

This follows a convention in include/uapi/linux/netfilter/*.h that
likely predates the introduction of uapi. A search for "Used
internally by the kernel" shows many more examples. I should not have
included filter.h, however. The common behavior when using pointers
to kernel-internal structures is to have a forward declaration. I suggest
making that change, instead of changing to void *. This avoids having
to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:

-#include <linux/filter.h>
 #include <linux/types.h>

 #define XT_BPF_MAX_NUM_INSTR   64

+struct sk_filter;
+
 struct xt_bpf_info {

I can send this as a separate patch to net-next, if that helps.

> You can just send me a patch to change it to void. It's an internal
> kernel pointer as the comment states. There is **no** way that
> userspace can lurk with that from iptables at all.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 6:00 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 07/25/2014 01:54 PM, Pablo Neira Ayuso wrote:
>>
>> On Fri, Jul 25, 2014 at 01:25:35PM +0200, Daniel Borkmann wrote:
>>>
>>> [ also Cc'ing Willem, Pablo ]
>>>
>>> On 07/25/2014 10:04 AM, Alexei Starovoitov wrote:
>>>>
>>>> 'sk_filter' name is used as 'struct sk_filter', function sk_filter() and
>>>> as variable 'sk_filter', which makes code hard to read.
>>>> Also it's easily confused with 'struct sock_filter'
>>>> Rename 'struct sk_filter' to 'struct bpf_prog' to clarify semantics and
>>>> align the name with generic BPF use model.
>>>
>>>
>>> Agreed, as we went for kernel/bpf/, renaming makes absolutely sense.
>>
>>
>> My nft socket filtering changes are accomodated into struct sk_filter,
>> and will still be, so I still need some generic name there...
>
>
> All the parts from filter.c which is BPF's core engine have been moved
> into kernel/bpf/ to get it ready for tracing et al, since there is not
> always a socket context anymore. The *whole* infrastructure around struct
> sk_filter is [e]BPF and used in non-net related contexts as well, whereas
> nft socket filtering is *only* for sockets. Due to the socket-only specific
> use case why doesn't it make more sense to have a union in struct sock
> around sk_filter (or however we name it) and only allow one of the two
> being loaded on a socket?

yep.
Adding nft specific things to struct sk_filter/bpf_prog is not correct,
since this struct is already part of seccomp and will be used
in net-less configurations. SK_RUN_FILTER() macro will also be
renamed into something like RUN_BPF_RPOG(). It's one and only
way to invoke eBPF programs. Adding nft selector cannot work,
since eBPF is used with generic context whereas nft is skb specific.
If you want to add nft filtering capabilities to sockets, you'd need
to add union around 'struct bpf_prog' inside 'struct sock', which will be
much cleaner way.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 6:53 AM, Willem de Bruijn <willemb@google.com> wrote:
>>> >alternative fix for xt_bpf.h could be to replace:
>>> >         /* only used in the kernel */
>>> >     struct sk_filter *filter __attribute__((aligned(8)));
>>> >with
>>> >         /* only used in the kernel */
>>> >     void *filter __attribute__((aligned(8)));
>>> >
>>> >but this 'void *' approach may further break broken userspace,
>>> >whereas the fix implemented here is more seamless.
>>>
>>> Yep, that's not good, 'struct sk_filter' should never have been in a uapi
>>> file actually.
>
> This follows a convention in include/uapi/linux/netfilter/*.h that
> likely predates the introduction of uapi. A search for "Used
> internally by the kernel" shows many more examples. I should not have
> included filter.h, however. The common behavior when using pointers
> to kernel-internal structures is to have a forward declaration. I suggest
> making that change, instead of changing to void *. This avoids having
> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:

that will not avoid typecast.
Either 'void *' approach or extra 'struct sk_filter;' approach, both need
type casts to 'struct bpf_prog' in xt_bpf.c
(because of SK_RUN_FILTER macro)
Therefore I prefer extra 'struct sk_filter;' approach.

> -#include <linux/filter.h>
>  #include <linux/types.h>
>
>  #define XT_BPF_MAX_NUM_INSTR   64
>
> +struct sk_filter;
> +
>  struct xt_bpf_info {
>
> I can send this as a separate patch to net-next, if that helps.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Willem de Bruijn]

>> This follows a convention in include/uapi/linux/netfilter/*.h that
>> likely predates the introduction of uapi. A search for "Used
>> internally by the kernel" shows many more examples. I should not have
>> included filter.h, however. The common behavior when using pointers
>> to kernel-internal structures is to have a forward declaration. I suggest
>> making that change, instead of changing to void *. This avoids having
>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
>
> that will not avoid typecast.
> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
> type casts to 'struct bpf_prog' in xt_bpf.c
> (because of SK_RUN_FILTER macro)
> Therefore I prefer extra 'struct sk_filter;' approach.

I hadn't noticed that your patch makes the same change that I
proposed. Nothing in userspace should touch that pointer, so it is
fine to change its type to struct bpf_prog* at the same time. No need
for typecasts.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 11:32 AM, Willem de Bruijn <willemb@google.com> wrote:
>>> This follows a convention in include/uapi/linux/netfilter/*.h that
>>> likely predates the introduction of uapi. A search for "Used
>>> internally by the kernel" shows many more examples. I should not have
>>> included filter.h, however. The common behavior when using pointers
>>> to kernel-internal structures is to have a forward declaration. I suggest
>>> making that change, instead of changing to void *. This avoids having
>>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
>>
>> that will not avoid typecast.
>> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
>> type casts to 'struct bpf_prog' in xt_bpf.c
>> (because of SK_RUN_FILTER macro)
>> Therefore I prefer extra 'struct sk_filter;' approach.
>
> I hadn't noticed that your patch makes the same change that I
> proposed. Nothing in userspace should touch that pointer, so it is
> fine to change its type to struct bpf_prog* at the same time. No need
> for typecasts.

really? I don't think it's a good idea to expose kernel struct type
to user space. How is it even going to compile?
#include <linux/filter.h> brings different files in kernel and in user space.
struct bpf_prog is undefined in user space and compiler will complain.
Adding 'struct bpf_prog;' will be ugly.
imo the lesser evil is adding 'struct sk_filter;' and doing type casts
in kernel.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Willem de Bruijn]

On Fri, Jul 25, 2014 at 2:43 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Fri, Jul 25, 2014 at 11:32 AM, Willem de Bruijn <willemb@google.com> wrote:
>>>> This follows a convention in include/uapi/linux/netfilter/*.h that
>>>> likely predates the introduction of uapi. A search for "Used
>>>> internally by the kernel" shows many more examples. I should not have
>>>> included filter.h, however. The common behavior when using pointers
>>>> to kernel-internal structures is to have a forward declaration. I suggest
>>>> making that change, instead of changing to void *. This avoids having
>>>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
>>>
>>> that will not avoid typecast.
>>> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
>>> type casts to 'struct bpf_prog' in xt_bpf.c
>>> (because of SK_RUN_FILTER macro)
>>> Therefore I prefer extra 'struct sk_filter;' approach.
>>
>> I hadn't noticed that your patch makes the same change that I
>> proposed. Nothing in userspace should touch that pointer, so it is
>> fine to change its type to struct bpf_prog* at the same time. No need
>> for typecasts.
>
> really? I don't think it's a good idea to expose kernel struct type
> to user space. How is it even going to compile?

a forward declaration.

> #include <linux/filter.h> brings different files in kernel and in user space.
> struct bpf_prog is undefined in user space and compiler will complain.
> Adding 'struct bpf_prog;' will be ugly.
> imo the lesser evil is adding 'struct sk_filter;' and doing type casts
> in kernel.

but the exact same argument applies to sk_filter. If that struct is
renamed everywhere else, then the result will only be more confusing.
A forward declaration is the standard workaround to all such cases in
include/uapi/linux/netfilter. See for instance xt_connlimit.h. This is
sufficient to allow userspace build to succeed, without exposing any
kernel structure detail. If you don't even want to leak the name, then
let's make it void *. Keeping a declaration for sk_filter, while
sk_filter is renamed everywhere else is the least good option, in my
opinion.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 11:50 AM, Willem de Bruijn <willemb@google.com> wrote:
> On Fri, Jul 25, 2014 at 2:43 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Fri, Jul 25, 2014 at 11:32 AM, Willem de Bruijn <willemb@google.com> wrote:
>>>>> This follows a convention in include/uapi/linux/netfilter/*.h that
>>>>> likely predates the introduction of uapi. A search for "Used
>>>>> internally by the kernel" shows many more examples. I should not have
>>>>> included filter.h, however. The common behavior when using pointers
>>>>> to kernel-internal structures is to have a forward declaration. I suggest
>>>>> making that change, instead of changing to void *. This avoids having
>>>>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
>>>>
>>>> that will not avoid typecast.
>>>> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
>>>> type casts to 'struct bpf_prog' in xt_bpf.c
>>>> (because of SK_RUN_FILTER macro)
>>>> Therefore I prefer extra 'struct sk_filter;' approach.
>>>
>>> I hadn't noticed that your patch makes the same change that I
>>> proposed. Nothing in userspace should touch that pointer, so it is
>>> fine to change its type to struct bpf_prog* at the same time. No need
>>> for typecasts.
>>
>> really? I don't think it's a good idea to expose kernel struct type
>> to user space. How is it even going to compile?
>
> a forward declaration.
>
>> #include <linux/filter.h> brings different files in kernel and in user space.
>> struct bpf_prog is undefined in user space and compiler will complain.
>> Adding 'struct bpf_prog;' will be ugly.
>> imo the lesser evil is adding 'struct sk_filter;' and doing type casts
>> in kernel.
>
> but the exact same argument applies to sk_filter. If that struct is
> renamed everywhere else, then the result will only be more confusing.
> A forward declaration is the standard workaround to all such cases in
> include/uapi/linux/netfilter. See for instance xt_connlimit.h. This is
> sufficient to allow userspace build to succeed, without exposing any
> kernel structure detail. If you don't even want to leak the name, then
> let's make it void *. Keeping a declaration for sk_filter, while
> sk_filter is renamed everywhere else is the least good option, in my
> opinion.

well, since you the author of this bit and you're ok with 'void *', I'm ok
with it too :) Just typecast in kernel is still needed because of
SK_RUN_FILTER() macro...

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 11:58 AM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Fri, Jul 25, 2014 at 11:50 AM, Willem de Bruijn <willemb@google.com> wrote:
>> On Fri, Jul 25, 2014 at 2:43 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>>> On Fri, Jul 25, 2014 at 11:32 AM, Willem de Bruijn <willemb@google.com> wrote:
>>>>>> This follows a convention in include/uapi/linux/netfilter/*.h that
>>>>>> likely predates the introduction of uapi. A search for "Used
>>>>>> internally by the kernel" shows many more examples. I should not have
>>>>>> included filter.h, however. The common behavior when using pointers
>>>>>> to kernel-internal structures is to have a forward declaration. I suggest
>>>>>> making that change, instead of changing to void *. This avoids having
>>>>>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
>>>>>
>>>>> that will not avoid typecast.
>>>>> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
>>>>> type casts to 'struct bpf_prog' in xt_bpf.c
>>>>> (because of SK_RUN_FILTER macro)
>>>>> Therefore I prefer extra 'struct sk_filter;' approach.
>>>>
>>>> I hadn't noticed that your patch makes the same change that I
>>>> proposed. Nothing in userspace should touch that pointer, so it is
>>>> fine to change its type to struct bpf_prog* at the same time. No need
>>>> for typecasts.
>>>
>>> really? I don't think it's a good idea to expose kernel struct type
>>> to user space. How is it even going to compile?
>>
>> a forward declaration.
>>
>>> #include <linux/filter.h> brings different files in kernel and in user space.
>>> struct bpf_prog is undefined in user space and compiler will complain.
>>> Adding 'struct bpf_prog;' will be ugly.
>>> imo the lesser evil is adding 'struct sk_filter;' and doing type casts
>>> in kernel.
>>
>> but the exact same argument applies to sk_filter. If that struct is
>> renamed everywhere else, then the result will only be more confusing.
>> A forward declaration is the standard workaround to all such cases in
>> include/uapi/linux/netfilter. See for instance xt_connlimit.h. This is
>> sufficient to allow userspace build to succeed, without exposing any
>> kernel structure detail. If you don't even want to leak the name, then
>> let's make it void *. Keeping a declaration for sk_filter, while
>> sk_filter is renamed everywhere else is the least good option, in my
>> opinion.
>
> well, since you the author of this bit and you're ok with 'void *', I'm ok
> with it too :) Just typecast in kernel is still needed because of
> SK_RUN_FILTER() macro...

just tried with 'void *', actually all three type casts are needed...

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Pablo Neira Ayuso]

On Fri, Jul 25, 2014 at 10:24:29AM -0700, Alexei Starovoitov wrote:
> On Fri, Jul 25, 2014 at 6:00 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> > On 07/25/2014 01:54 PM, Pablo Neira Ayuso wrote:
> >>
> >> On Fri, Jul 25, 2014 at 01:25:35PM +0200, Daniel Borkmann wrote:
> >>>
> >>> [ also Cc'ing Willem, Pablo ]
> >>>
> >>> On 07/25/2014 10:04 AM, Alexei Starovoitov wrote:
> >>>>
> >>>> 'sk_filter' name is used as 'struct sk_filter', function sk_filter() and
> >>>> as variable 'sk_filter', which makes code hard to read.
> >>>> Also it's easily confused with 'struct sock_filter'
> >>>> Rename 'struct sk_filter' to 'struct bpf_prog' to clarify semantics and
> >>>> align the name with generic BPF use model.
> >>>
> >>>
> >>> Agreed, as we went for kernel/bpf/, renaming makes absolutely sense.
> >>
> >>
> >> My nft socket filtering changes are accomodated into struct sk_filter,
> >> and will still be, so I still need some generic name there...
> >
> >
> > All the parts from filter.c which is BPF's core engine have been moved
> > into kernel/bpf/ to get it ready for tracing et al, since there is not
> > always a socket context anymore. The *whole* infrastructure around struct
> > sk_filter is [e]BPF and used in non-net related contexts as well, whereas
> > nft socket filtering is *only* for sockets. Due to the socket-only specific
> > use case why doesn't it make more sense to have a union in struct sock
> > around sk_filter (or however we name it) and only allow one of the two
> > being loaded on a socket?
> 
> yep.
> Adding nft specific things to struct sk_filter/bpf_prog is not correct,
> since this struct is already part of seccomp and will be used
> in net-less configurations. SK_RUN_FILTER() macro will also be
> renamed into something like RUN_BPF_RPOG(). It's one and only
> way to invoke eBPF programs. Adding nft selector cannot work,
> since eBPF is used with generic context whereas nft is skb specific.
> If you want to add nft filtering capabilities to sockets, you'd need
> to add union around 'struct bpf_prog' inside 'struct sock', which will be
> much cleaner way.

The struct sk_filter is almost providing the generic framework, it
just needs to be generalized, a quick layout for it:

struct sk_filter {
        struct sk_filter_cb     *cb;
        atomic_t                refcnt;
        struct rcu_head         head;
        char                    data[0]; /* here, you specific struct bpf_prog */
};

The refcnt is required sk_filter_{charge,uncharge,release}. The struct
rcu_head is also need from sk_filter_release().

struct sk_filter_cb {
        int type;
        struct module *me;
        void (*charge)(struct sock *sk, struct sk_filter *fp);
        void (*uncharge)(struct sock *sk, struct sk_filter *fp);
        unsigned int (*run_filter)(struct sk_filter *fp, struct sk_buff *skb);
};

We have to provide the register/unregister functions for the specific
callbacks depending on the socket filtering approach. But I'll have to
introduce this myself when I come up with the nft patches again.

So meanwhile, you should just encapsulate what really belongs to
struct bpf_prog, ie. size, bytecode, jitted, etc. and leave struct
sk_filter in place.

struct sk_filter {
        atomic_t                refcnt;
        struct rcu_head         head;
        u32                     len;
        struct bpf_prog         bpf_prog;
};

The len will go into struct bpf_prog once the generic infrastructure
above is introduced since the semantics (number of blocks) is
different from nft.

If you straight forward rename the entire structure, you'll take
things that are not specific from bpf such as refcnt and rcu_head.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Pablo Neira Ayuso]

On Fri, Jul 25, 2014 at 02:50:32PM -0400, Willem de Bruijn wrote:
> On Fri, Jul 25, 2014 at 2:43 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> > On Fri, Jul 25, 2014 at 11:32 AM, Willem de Bruijn <willemb@google.com> wrote:
> >>>> This follows a convention in include/uapi/linux/netfilter/*.h that
> >>>> likely predates the introduction of uapi. A search for "Used
> >>>> internally by the kernel" shows many more examples. I should not have
> >>>> included filter.h, however. The common behavior when using pointers
> >>>> to kernel-internal structures is to have a forward declaration. I suggest
> >>>> making that change, instead of changing to void *. This avoids having
> >>>> to add casts where xt_bpf_info is used in net/netfilter/xt_bpf.c:
> >>>
> >>> that will not avoid typecast.
> >>> Either 'void *' approach or extra 'struct sk_filter;' approach, both need
> >>> type casts to 'struct bpf_prog' in xt_bpf.c
> >>> (because of SK_RUN_FILTER macro)
> >>> Therefore I prefer extra 'struct sk_filter;' approach.
> >>
> >> I hadn't noticed that your patch makes the same change that I
> >> proposed. Nothing in userspace should touch that pointer, so it is
> >> fine to change its type to struct bpf_prog* at the same time. No need
> >> for typecasts.
> >
> > really? I don't think it's a good idea to expose kernel struct type
> > to user space. How is it even going to compile?
> 
> a forward declaration.
> 
> > #include <linux/filter.h> brings different files in kernel and in user space.
> > struct bpf_prog is undefined in user space and compiler will complain.
> > Adding 'struct bpf_prog;' will be ugly.
> > imo the lesser evil is adding 'struct sk_filter;' and doing type casts
> > in kernel.
> 
> but the exact same argument applies to sk_filter. If that struct is
> renamed everywhere else, then the result will only be more confusing.
> A forward declaration is the standard workaround to all such cases in
> include/uapi/linux/netfilter. See for instance xt_connlimit.h. This is
> sufficient to allow userspace build to succeed, without exposing any
> kernel structure detail. If you don't even want to leak the name, then
> let's make it void *. Keeping a declaration for sk_filter, while
> sk_filter is renamed everywhere else is the least good option, in my
> opinion.

Please, send me a patch to remove that include <net/filter.h> from the
uapi header and define struct sk_filter; so we save the typecast in
xt_bpf.c

The struct sk_filter; doesn't expose anything relevant since, even
assuming userspace knows the layout, it can *not* do anything useful
with that.

Thanks.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Fri, Jul 25, 2014 at 3:17 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jul 25, 2014 at 10:24:29AM -0700, Alexei Starovoitov wrote:
>> On Fri, Jul 25, 2014 at 6:00 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> > On 07/25/2014 01:54 PM, Pablo Neira Ayuso wrote:
>> >>
>> >> On Fri, Jul 25, 2014 at 01:25:35PM +0200, Daniel Borkmann wrote:
>> >>>
>> >>> [ also Cc'ing Willem, Pablo ]
>> >>>
>> >>> On 07/25/2014 10:04 AM, Alexei Starovoitov wrote:
>> >>>>
>> >>>> 'sk_filter' name is used as 'struct sk_filter', function sk_filter() and
>> >>>> as variable 'sk_filter', which makes code hard to read.
>> >>>> Also it's easily confused with 'struct sock_filter'
>> >>>> Rename 'struct sk_filter' to 'struct bpf_prog' to clarify semantics and
>> >>>> align the name with generic BPF use model.
>> >>>
>> >>>
>> >>> Agreed, as we went for kernel/bpf/, renaming makes absolutely sense.
>> >>
>> >>
>> >> My nft socket filtering changes are accomodated into struct sk_filter,
>> >> and will still be, so I still need some generic name there...
>> >
>> >
>> > All the parts from filter.c which is BPF's core engine have been moved
>> > into kernel/bpf/ to get it ready for tracing et al, since there is not
>> > always a socket context anymore. The *whole* infrastructure around struct
>> > sk_filter is [e]BPF and used in non-net related contexts as well, whereas
>> > nft socket filtering is *only* for sockets. Due to the socket-only specific
>> > use case why doesn't it make more sense to have a union in struct sock
>> > around sk_filter (or however we name it) and only allow one of the two
>> > being loaded on a socket?
>>
>> yep.
>> Adding nft specific things to struct sk_filter/bpf_prog is not correct,
>> since this struct is already part of seccomp and will be used
>> in net-less configurations. SK_RUN_FILTER() macro will also be
>> renamed into something like RUN_BPF_RPOG(). It's one and only
>> way to invoke eBPF programs. Adding nft selector cannot work,
>> since eBPF is used with generic context whereas nft is skb specific.
>> If you want to add nft filtering capabilities to sockets, you'd need
>> to add union around 'struct bpf_prog' inside 'struct sock', which will be
>> much cleaner way.
>
> The struct sk_filter is almost providing the generic framework, it
> just needs to be generalized, a quick layout for it:
>
> struct sk_filter {
>         struct sk_filter_cb     *cb;
>         atomic_t                refcnt;
>         struct rcu_head         head;
>         char                    data[0]; /* here, you specific struct bpf_prog */
> };
>
> The refcnt is required sk_filter_{charge,uncharge,release}. The struct
> rcu_head is also need from sk_filter_release().
>
> struct sk_filter_cb {
>         int type;
>         struct module *me;
>         void (*charge)(struct sock *sk, struct sk_filter *fp);
>         void (*uncharge)(struct sock *sk, struct sk_filter *fp);
>         unsigned int (*run_filter)(struct sk_filter *fp, struct sk_buff *skb);
> };

Pablo,

I don't think you understand the scope of BPF.
'struct module *'? to attach nft to sockets? ouch.
if you need to do it, go ahead, but please don't present such mess
as a 'generic' infra.
What you're proposing won't work even for classic bpf.
'struct sock *' and 'struct module *' are totally redundant and won't
work for seccomp, tracing and pretty much everything, but pure
socket filtering.
Somehow you think that 'struct sk_filter' is only used in sockets.
That is not the case. Just grep the git.
and arguing that 'struct sk_filter' needs to be generalized for nft
is shortsighted as minimum.
Every piece of code that is using 'struct sk_filter' knows that it's
dealing with bpf/ebpf, so renaming is not changing the facts,
but rather stating the obvious and making code easier to
understand.

> We have to provide the register/unregister functions for the specific
> callbacks depending on the socket filtering approach. But I'll have to
> introduce this myself when I come up with the nft patches again.

yes, please do introduce modules for nft only, since it doesn't
make sense for sockets and for bpf.

> So meanwhile, you should just encapsulate what really belongs to
> struct bpf_prog, ie. size, bytecode, jitted, etc. and leave struct
> sk_filter in place.
>
> struct sk_filter {
>         atomic_t                refcnt;
>         struct rcu_head         head;
>         u32                     len;
>         struct bpf_prog         bpf_prog;
> };
>
> The len will go into struct bpf_prog once the generic infrastructure
> above is introduced since the semantics (number of blocks) is
> different from nft.
>
> If you straight forward rename the entire structure, you'll take
> things that are not specific from bpf such as refcnt and rcu_head.

sorry Pablo, I don't think you understand what you're talking about.
refcnt and rcu_head are key parts of bpf/ebpf infra.
Programs are refcnt'ed not only by charging sockets. BPF is relying
on rcu for safe execution as well.
For example see my tracing filter patch that does:
void trace_filter_call_bpf(struct event_filter *filter, void *ctx)
{
        rcu_read_lock();
        SK_RUN_FILTER(filter->prog, ctx);
        rcu_read_unlock();
}
One eBPF program can be attached to multiple 'events' where event
is kprobe, tracepoint, syscall, etc The act of attaching increments
'struct sk_filter/bpf_prog -> refcnt'.
Every field of 'struct sk_filter' is used by BPF infra (including 'len'
and 'work') This struct is a definition of bpf program.
Seriously if you want to understand, just ask, but objecting due to
lack of understanding just silly.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Pablo Neira Ayuso]

On Sat, Jul 26, 2014 at 10:41:04PM -0700, Alexei Starovoitov wrote:
> On Fri, Jul 25, 2014 at 3:17 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > The struct sk_filter is almost providing the generic framework, it
> > just needs to be generalized, a quick layout for it:
> >
> > struct sk_filter {
> >         struct sk_filter_cb     *cb;
> >         atomic_t                refcnt;
> >         struct rcu_head         head;
> >         char                    data[0]; /* here, you specific struct bpf_prog */
> > };
> >
> > The refcnt is required sk_filter_{charge,uncharge,release}. The struct
> > rcu_head is also need from sk_filter_release().
> >
> > struct sk_filter_cb {
> >         int type;
> >         struct module *me;
> >         void (*charge)(struct sock *sk, struct sk_filter *fp);
> >         void (*uncharge)(struct sock *sk, struct sk_filter *fp);
> >         unsigned int (*run_filter)(struct sk_filter *fp, struct sk_buff *skb);
> > };
>
> Pablo,
>
> I don't think you understand the scope of BPF.
> 'struct module *'? to attach nft to sockets? ouch.

The idea is that there will be one sk_filter_cb per socket filtering
approach. The structure module is just there in case one of the
approach is loadable as kernel module, it's the typical code pattern
in the kernel. You can git grep for similar code.

> if you need to do it, go ahead, but please don't present such mess
> as a 'generic' infra.

This is extracted from one of your recent patches:

 void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
 {
-       atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
+       if (!fp->ebpf)
+               atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
        sk_filter_release(fp);
 }

 void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
 {
        atomic_inc(&fp->refcnt);
-       atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
+       if (!fp->ebpf)
+               atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
 }

Basically, that looks to me like two different socket filtering
approach. You only have to define the struct sock_filter_cb, set the
fields to point to the functions and register the approach in the
socket filtering engine. That will allow a simple way to look up the
filtering approach when loading the filter from userspace.

[...]
> What you're proposing won't work even for classic bpf.
> 'struct sock *' and 'struct module *' are totally redundant and won't
> work for seccomp, tracing and pretty much everything, but pure
> socket filtering.
> Somehow you think that 'struct sk_filter' is only used in sockets.
[...]
> > If you straight forward rename the entire structure, you'll take
> > things that are not specific from bpf such as refcnt and rcu_head.
>
> sorry Pablo, I don't think you understand what you're talking about.
> refcnt and rcu_head are key parts of bpf/ebpf infra.
> Programs are refcnt'ed not only by charging sockets. BPF is relying
> on rcu for safe execution as well.
> For example see my tracing filter patch that does:
> void trace_filter_call_bpf(struct event_filter *filter, void *ctx)
> {
>         rcu_read_lock();
>         SK_RUN_FILTER(filter->prog, ctx);
>         rcu_read_unlock();
> }
> One eBPF program can be attached to multiple 'events' where event
> is kprobe, tracepoint, syscall, etc The act of attaching increments
> 'struct sk_filter/bpf_prog -> refcnt'.

By quick git grepping you already can find clients of this that do not
need rcu / refcount: cls_bpf.c, net_cls.c, xt_bpf.c and
ptp_classifier.  Moreover, I only see the refcnt bumped from
sk_filter_charge(), I didn't find it neither in git nor in your
patches. I don't think rcu_head and refcnt are really part of the
filter *in any case*, they just provide the way to link/unlink objects
in a safe way in some situations.

By renaming this, you're not fixing up things the semantics. It seems
to me you just want to find a quick path to solve inconsistencies in
your code.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Mon, 28 Jul 2014 23:45:52 +0200

> By renaming this, you're not fixing up things the semantics. It seems
> to me you just want to find a quick path to solve inconsistencies in
> your code.

Agreed, this looks just like messing around with naming to me.

But to the original issue, that of xt_bpf, I wonder about a few things:

1) If we have a kernel pointer embedded in a user provided datastructure,
   what takes care of 32-bit compat applications uploading xt_bpf rules
   on a 64-bit kernel?  Won't the size be wrong or does it not matter
   and is in some way helped by that 8-byte alignment thing there?

2) The user can't care about the type of "filter" in xt_bpf_info, so
   we can use whatever name we want for the type.

   Therefore you can just do something like:


struct bpf_prog;

struct xt_bpf_info {
	__u16 bpf_program_num_elem;
	struct sock_filter bpf_program[XT_BPF_MAX_NUM_INSTR];

	/* only used in the kernel */
	struct bpf_prog *filter __attribute__((aligned(8)));
};

   and then you won't need any casting.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[Alexei Starovoitov]

On Mon, Jul 28, 2014 at 2:45 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > struct sk_filter_cb {
>> >         int type;
>> >         struct module *me;
>> >         void (*charge)(struct sock *sk, struct sk_filter *fp);
>> >         void (*uncharge)(struct sock *sk, struct sk_filter *fp);
>> >         unsigned int (*run_filter)(struct sk_filter *fp, struct sk_buff *skb);
>> > };
>>
>> Pablo,
>>
>> I don't think you understand the scope of BPF.
>> 'struct module *'? to attach nft to sockets? ouch.
>
> The idea is that there will be one sk_filter_cb per socket filtering
> approach. The structure module is just there in case one of the
> approach is loadable as kernel module, it's the typical code pattern
> in the kernel. You can git grep for similar code.

socket filtering is available to unprivileged users.
So you're proposing to let them increment refcnt of modules?!
That's not secure.
You're also ignoring kernel address leak issue of xt_bpf
pointed out earlier. Userspace should not be able to see kernel
address inside 'struct xt_bpf_info'.
xtables need some sort of scrub_matchinfo callback to clean it
before passing back to user.

> This is extracted from one of your recent patches:
>
>  void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
>  {
> -       atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> +       if (!fp->ebpf)
> +               atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
>         sk_filter_release(fp);
>  }
>
>  void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
>  {
>         atomic_inc(&fp->refcnt);
> -       atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
> +       if (!fp->ebpf)
> +               atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
>  }
>
> Basically, that looks to me like two different socket filtering
> approach. You only have to define the struct sock_filter_cb, set the
> fields to point to the functions and register the approach in the
> socket filtering engine. That will allow a simple way to look up the
> filtering approach when loading the filter from userspace.

You're taking things out of context. Quoted code is from patch #9.
This rename is patch #0
Once it's all properly named it will look like:
void sk_filter_charge(struct sock *sk, struct bpf_prog *fp)
{
  atomic_inc(&fp->refcnt);
  if (!fp->ebpf)
    atomic_add(bpf_filter_size(fp->len), &sk->sk_omem_alloc);
}

so it's one sk_filter_charge() function to deal with two variants of
bpf_prog (native ebpf and converted to ebpf).
In all cases the programs inside are in ebpf isa.
fp->ebpf flag means that program arrived from userspace as
native ebpf  (and not was a result of conversion from classic).
So splitting this function in two would not make sense at all.

> By quick git grepping you already can find clients of this that do not
> need rcu / refcount: cls_bpf.c, net_cls.c, xt_bpf.c and

you can also see a lot of clients that don't use jit and 'work' field
is unused. That doesn't mean that these flags should be in
different structure.

> ptp_classifier.  Moreover, I only see the refcnt bumped from
> sk_filter_charge(), I didn't find it neither in git nor in your
> patches.

in the patch #9 that you already quoted there is this part:

+/* called from sk_attach_filter_ebpf() or from tracing filter attach
+ * pairs with
+ * sk_detach_filter()->sk_filter_uncharge()->sk_filter_release()
+ * or with
+ * sk_unattached_filter_destroy()->sk_filter_release()
+ */
+struct sk_filter *bpf_prog_get(u32 ufd)
+{
+       struct fd f = fdget(ufd);
+       struct sk_filter *prog;
+
+       prog = get_prog(f);
+
+       if (IS_ERR(prog))
+               return prog;
+
+       atomic_inc(&prog->refcnt);
+       fdput(f);
+       return prog;
+}

see how 'native ebpf' reuses all existing structs?
Only renaming of 'struct sk_filter' and 'sk_filter_release()' is missing.
sk_filter_uncharge() name should stay as-is, since it is working
on 'struct sock*', whereas sk_filter_release() is working on bpf prog,
so should be renamed as well into 'bpf_prog_release()'

> I don't think rcu_head and refcnt are really part of the
> filter *in any case*, they just provide the way to link/unlink objects
> in a safe way in some situations.

what is a program then? a sequence of instructions only? if so,
what do you call a structure that carries refcnt, rcu, work and flags?
Using your logic task_struct should only have fields that describe
the task and all auxiliary fields should be moved to different struct?

> By renaming this, you're not fixing up things the semantics. It seems
> to me you just want to find a quick path to solve inconsistencies in
> your code.

Please point to inconsistencies.
It sounds to me that you're arguing only because you think that this
renaming will make it harder for you to add nft to socket filtering.
That is not the case. sk_filter_cb can be added later.

> Agreed, this looks just like messing around with naming to me.

guys, I don't see an alternative to renaming. All fields of 'struct sk_filter'
are used and needed to be part of bpf program.
Just look at 'bpf: expand BPF syscall with program load/unload' patch:
https://git.kernel.org/cgit/linux/kernel/git/ast/bpf.git/commit/?id=7a5b36ee1cb57e5fcb3e2414645dc9f5fdc3c404

There I blend 'native epbf' into 'struct sk_filter' like:
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -30,12 +30,17 @@ struct sock_fprog_kern {
 struct sk_buff;
 struct sock;
 struct seccomp_data;
+struct bpf_prog_info;

 struct sk_filter {
        atomic_t                refcnt;
        u32                     jited:1,        /* Is our filter JIT'ed? */
-                               len:31;         /* Number of filter blocks */
-       struct sock_fprog_kern  *orig_prog;     /* Original BPF program */
+                               ebpf:1,         /* Is it eBPF program ? */
+                               len:30;         /* Number of filter blocks */
+       union {
+               struct sock_fprog_kern  *orig_prog;     /* Original
BPF program */
+               struct bpf_prog_info    *info;
+       };
        struct rcu_head         rcu;
        unsigned int            (*bpf_func)(const struct sk_buff *skb,
                                            const struct bpf_insn *filter);

where 'struct bpf_prog_info' carries additional info about bpf maps, etc:
that is ideal code reuse. All existing and new ebpf infra and structures
are common and shared. Only 'struct sk_filter' name doesn't make sense.

The alternative is to copy paste all of 'struct sk_filter' fields into new
structure ? You seriously think it's a better option?

I cannot see how the arguments about some future sk_filter_cb apply here.
When we get to the point of having multiple socket filtering engines,
we can add new 'struct sk_filter' and callbacks if necessary.
Today 'struct sk_filter' is all about bpf. Keep calling it something else
but 'bpf_prog' just denying the reality.

Re: [PATCH net-next] net: filter: rename 'struct sk_filter' to 'struct bpf_prog'

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Mon, 28 Jul 2014 18:12:05 -0700

> On Mon, Jul 28, 2014 at 2:45 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>> > struct sk_filter_cb {
>>> >         int type;
>>> >         struct module *me;
>>> >         void (*charge)(struct sock *sk, struct sk_filter *fp);
>>> >         void (*uncharge)(struct sock *sk, struct sk_filter *fp);
>>> >         unsigned int (*run_filter)(struct sk_filter *fp, struct sk_buff *skb);
>>> > };
>>>
>>> Pablo,
>>>
>>> I don't think you understand the scope of BPF.
>>> 'struct module *'? to attach nft to sockets? ouch.
>>
>> The idea is that there will be one sk_filter_cb per socket filtering
>> approach. The structure module is just there in case one of the
>> approach is loadable as kernel module, it's the typical code pattern
>> in the kernel. You can git grep for similar code.
> 
> socket filtering is available to unprivileged users.
> So you're proposing to let them increment refcnt of modules?!
> That's not secure.

It's impossible to avoid, and really is nothing new.

Users can open sockets, and that holds a reference to the module
implementing that protocol.  Is that not secure too?

This discussion is degenerating into nonsense, please stop ignoring
Pablo's core points.

Thanks.