| Message ID | 1360029450-6673-1-git-send-email-gaofeng@cn.fujitsu.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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
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); } }
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(-)