Patchwork ulog: add protection when remove ipt_ULOG

login
register
mail settings
Submitter Gao feng
Date Feb. 5, 2013, 1:57 a.m.
Message ID <1360029450-6673-1-git-send-email-gaofeng@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/218131/
State Not Applicable
Headers show

Comments

Gao feng - Feb. 5, 2013, 1:57 a.m.
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(-)
Pablo Neira - Feb. 7, 2013, 6:27 p.m.
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
--
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
Gao feng - Feb. 18, 2013, 3:57 a.m.
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?
--
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
Pablo Neira - Feb. 18, 2013, 4:52 p.m.
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.
--
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
Gao feng - Feb. 19, 2013, 1:09 a.m.
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!
--
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
Pablo Neira - March 15, 2013, 11:49 a.m.
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.
--
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

Patch

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);
 	}
 }