[PATCH] ulog: add protection when remove ipt_ULOG

[PATCH] ulog: add protection when remove ipt_ULOG

[Gao feng]

We should add a lock protection when we free the skb,
because it maybe used by ipt_ulog_packet right now.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/ipv4/netfilter/ipt_ULOG.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index b5ef3cb..b390002 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -430,11 +430,12 @@ static void __exit ulog_tg_exit(void)
 			pr_debug("timer was pending, deleting\n");
 			del_timer(&ub->timer);
 		}
-
+		spin_lock_bh(&ulog_lock);
 		if (ub->skb) {
 			kfree_skb(ub->skb);
 			ub->skb = NULL;
 		}
+		spin_unlock_bh(&ulog_lock);
 	}
 }
 
-- 
1.7.11.7

[PATCH 2/2] netfilter: ipt_ULOG: make spinlock per nlgroup

[Gao feng]

This patch uses the spin lock per group instead of
global spin lock,just like ebt_ulog.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/ipv4/netfilter/ipt_ULOG.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index b390002..09a03e4 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -76,12 +76,12 @@ typedef struct {
 	struct nlmsghdr *lastnlh;	/* netlink header of last msg in skb */
 	struct sk_buff *skb;		/* the pre-allocated skb */
 	struct timer_list timer;	/* the timer function */
+	spinlock_t lock;		/* the pre-queue lock */
 } ulog_buff_t;
 
 static ulog_buff_t ulog_buffers[ULOG_MAXNLGROUPS];	/* array of buffers */
 
 static struct sock *nflognl;		/* our socket */
-static DEFINE_SPINLOCK(ulog_lock);	/* spinlock */
 
 /* send one ulog_buff_t to userspace */
 static void ulog_send(unsigned int nlgroupnum)
@@ -120,9 +120,9 @@ static void ulog_timer(unsigned long data)
 
 	/* lock to protect against somebody modifying our structure
 	 * from ipt_ulog_target at the same time */
-	spin_lock_bh(&ulog_lock);
+	spin_lock_bh(&ulog_buffers[data].lock);
 	ulog_send(data);
-	spin_unlock_bh(&ulog_lock);
+	spin_unlock_bh(&ulog_buffers[data].lock);
 }
 
 static struct sk_buff *ulog_alloc_skb(unsigned int size)
@@ -178,7 +178,7 @@ static void ipt_ulog_packet(unsigned int hooknum,
 
 	ub = &ulog_buffers[groupnum];
 
-	spin_lock_bh(&ulog_lock);
+	spin_lock_bh(&ub->lock);
 
 	if (!ub->skb) {
 		if (!(ub->skb = ulog_alloc_skb(size)))
@@ -265,13 +265,13 @@ static void ipt_ulog_packet(unsigned int hooknum,
 		ulog_send(groupnum);
 	}
 out_unlock:
-	spin_unlock_bh(&ulog_lock);
+	spin_unlock_bh(&ub->lock);
 
 	return;
 
 alloc_failure:
 	pr_debug("Error building netlink message\n");
-	spin_unlock_bh(&ulog_lock);
+	spin_unlock_bh(&ub->lock);
 }
 
 static unsigned int
@@ -393,8 +393,10 @@ static int __init ulog_tg_init(void)
 	}
 
 	/* initialize ulog_buffers */
-	for (i = 0; i < ULOG_MAXNLGROUPS; i++)
+	for (i = 0; i < ULOG_MAXNLGROUPS; i++) {
 		setup_timer(&ulog_buffers[i].timer, ulog_timer, i);
+		spin_lock_init(&ulog_buffers[i].lock);
+	}
 
 	nflognl = netlink_kernel_create(&init_net, NETLINK_NFLOG, &cfg);
 	if (!nflognl)
@@ -430,12 +432,12 @@ static void __exit ulog_tg_exit(void)
 			pr_debug("timer was pending, deleting\n");
 			del_timer(&ub->timer);
 		}
-		spin_lock_bh(&ulog_lock);
+		spin_lock_bh(&ub->lock);
 		if (ub->skb) {
 			kfree_skb(ub->skb);
 			ub->skb = NULL;
 		}
-		spin_unlock_bh(&ulog_lock);
+		spin_unlock_bh(&ub->lock);
 	}
 }
 
-- 
1.7.11.7

Re: [PATCH] ulog: add protection when remove ipt_ULOG

[Pablo Neira Ayuso]

Hi Gao,

On Tue, Feb 05, 2013 at 09:57:30AM +0800, Gao feng wrote:
> We should add a lock protection when we free the skb,
> because it maybe used by ipt_ulog_packet right now.

Did you hit a reproducible crash?

I think this is very unlikely to happen. The removal of the module
happens in user-context and the entire path to build and deliver the
skb to user-space is protected is under spin_lock_bh, so scheduling
is not possible.

> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/ipv4/netfilter/ipt_ULOG.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
> index b5ef3cb..b390002 100644
> --- a/net/ipv4/netfilter/ipt_ULOG.c
> +++ b/net/ipv4/netfilter/ipt_ULOG.c
> @@ -430,11 +430,12 @@ static void __exit ulog_tg_exit(void)
>  			pr_debug("timer was pending, deleting\n");
>  			del_timer(&ub->timer);
>  		}
> -
> +		spin_lock_bh(&ulog_lock);
>  		if (ub->skb) {
>  			kfree_skb(ub->skb);
>  			ub->skb = NULL;
>  		}
> +		spin_unlock_bh(&ulog_lock);
>  	}
>  }
>  
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ulog: add protection when remove ipt_ULOG

[Gao feng]

Hi Pablo,

On 2013/02/08 02:27, Pablo Neira Ayuso wrote:
> Hi Gao,
> 
> On Tue, Feb 05, 2013 at 09:57:30AM +0800, Gao feng wrote:
>> We should add a lock protection when we free the skb,
>> because it maybe used by ipt_ulog_packet right now.
> 
> Did you hit a reproducible crash?
> 

I didn't.
I looked at the ebt_ulog.c and found ebt_ulog_fini
uses the spin lock to protect the ulog_buff's skb.


> I think this is very unlikely to happen. The removal of the module
> happens in user-context and the entire path to build and deliver the
> skb to user-space is protected is under spin_lock_bh, so scheduling
> is not possible.
>

Doesn't spin_lock_bh only disable local cpu's bottom-half?
the task that remove the modules can run on other cpus at the same time.
I'm wrong?

Re: [PATCH] ulog: add protection when remove ipt_ULOG

[Pablo Neira Ayuso]

On Mon, Feb 18, 2013 at 11:57:36AM +0800, Gao feng wrote:
[...]
> > I think this is very unlikely to happen. The removal of the module
> > happens in user-context and the entire path to build and deliver the
> > skb to user-space is protected is under spin_lock_bh, so scheduling
> > is not possible.
> 
> Doesn't spin_lock_bh only disable local cpu's bottom-half?
> the task that remove the modules can run on other cpus at the same time.
> I'm wrong?

That's right. But that will not happen since the removal of ipt_ULOG
is protected by the module refcount, which is bumped for each iptables
rule. So, you have to remove all rules using the ULOG target first to
be able to rmmod that module, but then there is no chance to race with
packets.

Re: [PATCH] ulog: add protection when remove ipt_ULOG

[Gao feng]

On 2013/02/19 00:52, Pablo Neira Ayuso wrote:
> On Mon, Feb 18, 2013 at 11:57:36AM +0800, Gao feng wrote:
> [...]
>>> I think this is very unlikely to happen. The removal of the module
>>> happens in user-context and the entire path to build and deliver the
>>> skb to user-space is protected is under spin_lock_bh, so scheduling
>>> is not possible.
>>
>> Doesn't spin_lock_bh only disable local cpu's bottom-half?
>> the task that remove the modules can run on other cpus at the same time.
>> I'm wrong?
> 
> That's right. But that will not happen since the removal of ipt_ULOG
> is protected by the module refcount, which is bumped for each iptables
> rule. So, you have to remove all rules using the ULOG target first to
> be able to rmmod that module, but then there is no chance to race with
> packets.

this calltrack doesn't add the refcount of moudule.
trace_packet->nf_log_packet->logger->logfn(ipt_logfn).
But when removing module,we call nf_log_unregister and
we can make sure only ulog_tg_exit uses ulog_buffer->skb,
So it's safe to don't add spin lock protect here.

I will send a v2 patchset to remove the spin lock protect
in ebt_ulog module.

Thanks!

Re: [PATCH] ulog: add protection when remove ipt_ULOG

[Pablo Neira Ayuso]

On Tue, Feb 05, 2013 at 09:57:30AM +0800, Gao feng wrote:
> We should add a lock protection when we free the skb,
> because it maybe used by ipt_ulog_packet right now.

applied, thanks.

Re: [PATCH 2/2] netfilter: ipt_ULOG: make spinlock per nlgroup

[Pablo Neira Ayuso]

On Tue, Feb 05, 2013 at 10:48:37AM +0800, Gao feng wrote:
> This patch uses the spin lock per group instead of
> global spin lock,just like ebt_ulog.

I'd prefer if we focus on NFLOG, that has been around since quite for
a while. ULOG should be scheduled for removal, we can add some printk
in the checkentry path to let people know.

Re: [PATCH 2/2] netfilter: ipt_ULOG: make spinlock per nlgroup

[Gao feng]

On 2013/03/15 19:51, Pablo Neira Ayuso wrote:
> On Tue, Feb 05, 2013 at 10:48:37AM +0800, Gao feng wrote:
>> This patch uses the spin lock per group instead of
>> global spin lock,just like ebt_ulog.
> 
> I'd prefer if we focus on NFLOG, that has been around since quite for
> a while. ULOG should be scheduled for removal, we can add some printk
> in the checkentry path to let people know.
> 

Ok,let's just drop this patch. :)

Thanks!