[PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[Pablo Neira Ayuso]

In e6f30c7 ("netfilter: x_tables: add xt_bpf match"), the internal
linux/filter.h header slipped through in the user exposed xt_bpf.h
header as included file.

Fix this by defining struct sk_filter; so we skip the casting in
kernelspace. This is safe since userspace has no way to lurk with
that internal pointer.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
@David: This patch is very small and it can probably go into net, but it's
        quite late for changes in -rc7 probably. Your call :-). Thanks.

 include/uapi/linux/netfilter/xt_bpf.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/xt_bpf.h b/include/uapi/linux/netfilter/xt_bpf.h
index 5dda450..93fca65 100644
--- a/include/uapi/linux/netfilter/xt_bpf.h
+++ b/include/uapi/linux/netfilter/xt_bpf.h
@@ -1,11 +1,12 @@
 #ifndef _XT_BPF_H
 #define _XT_BPF_H
 
-#include <linux/filter.h>
 #include <linux/types.h>
 
 #define XT_BPF_MAX_NUM_INSTR	64
 
+struct sk_filter;
+
 struct xt_bpf_info {
 	__u16 bpf_program_num_elem;
 	struct sock_filter bpf_program[XT_BPF_MAX_NUM_INSTR];
-- 
1.7.10.4

[PATCH net-next 2/2] net: filter: don't release unattached filter through call_rcu()

[Pablo Neira Ayuso]

sk_unattached_filter_destroy() does not always need to release the
filter object via rcu. Since this filter is never attached to the
socket, the caller should be responsible for releasing the filter
in a safe way, which may not necessarily imply rcu.

This is a short summary of clients of this function:

1) xt_bpf.c and cls_bpf.c use the bpf matchers from rules, these rules
   are removed from the packet path before the filter is released. Thus,
   the framework makes sure the filter is safely removed.

2) In the ppp driver, the ppp_lock ensures serialization between the
   xmit and filter attachment/detachment path. This doesn't use rcu
   so deferred release via rcu makes no sense.

3) In the isdn/ppp driver, it is called from isdn_ppp_release()
   the isdn_ppp_ioctl(). This driver uses mutex and spinlocks, no rcu.
   Thus, deferred rcu makes no sense to me either, the deferred releases
   may be just masking the effects of wrong locking strategy, which
   should be fixed in the driver itself.

4) In the team driver, this is the only place where the rcu
   synchronization with unattached filter is used. Therefore, this
   patch introduces synchronize_rcu() which is called from the
   genetlink path to make sure the filter doesn't go away while packets
   are still walking over it. I think we can revisit this once struct
   bpf_prog (that only wraps specific bpf code bits) is in place, then
   add some specific struct rcu_head in the scope of the team driver if
   Jiri thinks this is needed.

Deferred rcu release for unattached filters was originally introduced
in 302d663 ("filter: Allow to create sk-unattached filters").

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/team/team_mode_loadbalance.c |    6 +++++-
 net/core/filter.c                        |   11 ++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/team/team_mode_loadbalance.c b/drivers/net/team/team_mode_loadbalance.c
index a58dfeb..7106f34 100644
--- a/drivers/net/team/team_mode_loadbalance.c
+++ b/drivers/net/team/team_mode_loadbalance.c
@@ -293,11 +293,15 @@ static int lb_bpf_func_set(struct team *team, struct team_gsetter_ctx *ctx)
 		__fprog_destroy(lb_priv->ex->orig_fprog);
 		orig_fp = rcu_dereference_protected(lb_priv->fp,
 						lockdep_is_held(&team->lock));
-		sk_unattached_filter_destroy(orig_fp);
 	}
 
 	rcu_assign_pointer(lb_priv->fp, fp);
 	lb_priv->ex->orig_fprog = fprog;
+
+	if (orig_fp) {
+		synchronize_rcu();
+		sk_unattached_filter_destroy(orig_fp);
+	}
 	return 0;
 }
 
diff --git a/net/core/filter.c b/net/core/filter.c
index f3b2d5e..42c1944 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -841,6 +841,12 @@ static void sk_release_orig_filter(struct sk_filter *fp)
 	}
 }
 
+static void __sk_filter_release(struct sk_filter *fp)
+{
+	sk_release_orig_filter(fp);
+	sk_filter_free(fp);
+}
+
 /**
  * 	sk_filter_release_rcu - Release a socket filter by rcu_head
  *	@rcu: rcu_head that contains the sk_filter to free
@@ -849,8 +855,7 @@ static void sk_filter_release_rcu(struct rcu_head *rcu)
 {
 	struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
 
-	sk_release_orig_filter(fp);
-	sk_filter_free(fp);
+	__sk_filter_release(fp);
 }
 
 /**
@@ -1050,7 +1055,7 @@ EXPORT_SYMBOL_GPL(sk_unattached_filter_create);
 
 void sk_unattached_filter_destroy(struct sk_filter *fp)
 {
-	sk_filter_release(fp);
+	__sk_filter_release(fp);
 }
 EXPORT_SYMBOL_GPL(sk_unattached_filter_destroy);
 
-- 
1.7.10.4

Re: [PATCH net-next 2/2] net: filter: don't release unattached filter through call_rcu()

[Alexei Starovoitov]

On Tue, Jul 29, 2014 at 8:36 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> sk_unattached_filter_destroy() does not always need to release the
> filter object via rcu. Since this filter is never attached to the
> socket, the caller should be responsible for releasing the filter
> in a safe way, which may not necessarily imply rcu.
>
> This is a short summary of clients of this function:
>
> 1) xt_bpf.c and cls_bpf.c use the bpf matchers from rules, these rules
>    are removed from the packet path before the filter is released. Thus,
>    the framework makes sure the filter is safely removed.
>
> 2) In the ppp driver, the ppp_lock ensures serialization between the
>    xmit and filter attachment/detachment path. This doesn't use rcu
>    so deferred release via rcu makes no sense.
>
> 3) In the isdn/ppp driver, it is called from isdn_ppp_release()
>    the isdn_ppp_ioctl(). This driver uses mutex and spinlocks, no rcu.
>    Thus, deferred rcu makes no sense to me either, the deferred releases
>    may be just masking the effects of wrong locking strategy, which
>    should be fixed in the driver itself.
>
> 4) In the team driver, this is the only place where the rcu
>    synchronization with unattached filter is used. Therefore, this
>    patch introduces synchronize_rcu() which is called from the
>    genetlink path to make sure the filter doesn't go away while packets
>    are still walking over it. I think we can revisit this once struct
>    bpf_prog (that only wraps specific bpf code bits) is in place, then
>    add some specific struct rcu_head in the scope of the team driver if
>    Jiri thinks this is needed.

Correct.
There are also:
5) ptp_classifer - which is ok as well, since there is no filter_destroy.

6) seccomp - there I think it's also ok, since filter is called from syscall
and filter freeing is done on task exit

Re: [PATCH net-next 2/2] net: filter: don't release unattached filter through call_rcu()

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 29 Jul 2014 17:36:28 +0200

> sk_unattached_filter_destroy() does not always need to release the
> filter object via rcu. Since this filter is never attached to the
> socket, the caller should be responsible for releasing the filter
> in a safe way, which may not necessarily imply rcu.
> 
> This is a short summary of clients of this function:
> 
> 1) xt_bpf.c and cls_bpf.c use the bpf matchers from rules, these rules
>    are removed from the packet path before the filter is released. Thus,
>    the framework makes sure the filter is safely removed.
> 
> 2) In the ppp driver, the ppp_lock ensures serialization between the
>    xmit and filter attachment/detachment path. This doesn't use rcu
>    so deferred release via rcu makes no sense.
> 
> 3) In the isdn/ppp driver, it is called from isdn_ppp_release()
>    the isdn_ppp_ioctl(). This driver uses mutex and spinlocks, no rcu.
>    Thus, deferred rcu makes no sense to me either, the deferred releases
>    may be just masking the effects of wrong locking strategy, which
>    should be fixed in the driver itself.
> 
> 4) In the team driver, this is the only place where the rcu
>    synchronization with unattached filter is used. Therefore, this
>    patch introduces synchronize_rcu() which is called from the
>    genetlink path to make sure the filter doesn't go away while packets
>    are still walking over it. I think we can revisit this once struct
>    bpf_prog (that only wraps specific bpf code bits) is in place, then
>    add some specific struct rcu_head in the scope of the team driver if
>    Jiri thinks this is needed.
> 
> Deferred rcu release for unattached filters was originally introduced
> in 302d663 ("filter: Allow to create sk-unattached filters").
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Also applied, thanks.

Re: [PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[Willem de Bruijn]

> In e6f30c7 ("netfilter: x_tables: add xt_bpf match"), the internal
> linux/filter.h header slipped through in the user exposed xt_bpf.h
> header as included file.

is that true? #include <linux/filter.h> should include
include/uapi/linux/filter.h in userspace builds.

>
> +struct sk_filter;
> +
>  struct xt_bpf_info {
>         __u16 bpf_program_num_elem;
>         struct sock_filter bpf_program[XT_BPF_MAX_NUM_INSTR];

I think include/uapi/linux/filter.h is still needed for the definition
of struct sock_filter.

The uapi file does not declare sk_filter, so we do need to add the
forward declaration. Thanks, Pablo.

Re: [PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[Pablo Neira Ayuso]

On Tue, Jul 29, 2014 at 11:43:57AM -0400, Willem de Bruijn wrote:
> > In e6f30c7 ("netfilter: x_tables: add xt_bpf match"), the internal
> > linux/filter.h header slipped through in the user exposed xt_bpf.h
> > header as included file.
> 
> is that true? #include <linux/filter.h> should include
> include/uapi/linux/filter.h in userspace builds.

You're right. I'm going to send a v2.

We should also cache the linux/filter.h header in the iptables
sources, would you send a patch for that? Thanks.

Re: [PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[Willem de Bruijn]

On Tue, Jul 29, 2014 at 12:05 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jul 29, 2014 at 11:43:57AM -0400, Willem de Bruijn wrote:
>> > In e6f30c7 ("netfilter: x_tables: add xt_bpf match"), the internal
>> > linux/filter.h header slipped through in the user exposed xt_bpf.h
>> > header as included file.
>>
>> is that true? #include <linux/filter.h> should include
>> include/uapi/linux/filter.h in userspace builds.
>
> You're right. I'm going to send a v2.

Thanks for fixing my bug. I agree that it's better to do this
independent of the sk_filter/bpf_prog patch.

> We should also cache the linux/filter.h header in the iptables
> sources, would you send a patch for that? Thanks.

Will do.  Should I just import the latest version of the file, or is
there a sync policy, such as that all kernel headers come from the
same kernel release?

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

Re: [PATCH net-next 1/2] netfilter: xt_bpf: don't include linux/filter.h from uapi header

[Pablo Neira Ayuso]

On Tue, Jul 29, 2014 at 12:35:00PM -0400, Willem de Bruijn wrote:
> On Tue, Jul 29, 2014 at 12:05 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Jul 29, 2014 at 11:43:57AM -0400, Willem de Bruijn wrote:
> >> > In e6f30c7 ("netfilter: x_tables: add xt_bpf match"), the internal
> >> > linux/filter.h header slipped through in the user exposed xt_bpf.h
> >> > header as included file.
> >>
> >> is that true? #include <linux/filter.h> should include
> >> include/uapi/linux/filter.h in userspace builds.
> >
> > You're right. I'm going to send a v2.
> 
> Thanks for fixing my bug. I agree that it's better to do this
> independent of the sk_filter/bpf_prog patch.
> 
> > We should also cache the linux/filter.h header in the iptables
> > sources, would you send a patch for that? Thanks.
> 
> Will do.  Should I just import the latest version of the file, or is
> there a sync policy, such as that all kernel headers come from the
> same kernel release?

Right. The kernel header which is available after make headers_install.
Thanks.