| Message ID | 1348635140-20225-1-git-send-email-gaofeng@cn.fujitsu.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Sep 26, 2012 at 12:52:10PM +0800, Gao feng wrote: > + > int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, > const struct nlmsghdr *nlh, > struct netlink_dump_control *control) > @@ -1786,6 +1794,7 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, > cb->done = control->done; > cb->nlh = nlh; > cb->data = control->data; > + cb->module = control->module; > cb->min_dump_alloc = control->min_dump_alloc; > atomic_inc(&skb->users); > cb->skb = skb; > @@ -1796,19 +1805,27 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, > return -ECONNREFUSED; > } > nlk = nlk_sk(sk); > - /* A dump is in progress... */ > + > mutex_lock(nlk->cb_mutex); > + /* A dump is in progress... */ > if (nlk->cb) { > mutex_unlock(nlk->cb_mutex); > netlink_destroy_callback(cb); > - sock_put(sk); > - return -EBUSY; > + ret = -EBUSY; > + goto out; > + } > + /* add reference of module witch cb->dump belone to */ > + if (cb->module && !try_module_get(cb->module)) { > + mutex_unlock(nlk->cb_mutex); > + ret = -EPROTONOSUPPORT; > + goto out; Looks like you leak the allocated netlink_callback here. You should call netlink_destroy_callback() before you exit. > } > + > nlk->cb = cb; > mutex_unlock(nlk->cb_mutex); > > ret = netlink_dump(sk); > - > +out: > sock_put(sk); > > if (ret) > -- -- 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
于 2012年09月26日 13:41, Steffen Klassert 写道: > On Wed, Sep 26, 2012 at 12:52:10PM +0800, Gao feng wrote: >> + >> int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, >> const struct nlmsghdr *nlh, >> struct netlink_dump_control *control) >> @@ -1786,6 +1794,7 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, >> cb->done = control->done; >> cb->nlh = nlh; >> cb->data = control->data; >> + cb->module = control->module; >> cb->min_dump_alloc = control->min_dump_alloc; >> atomic_inc(&skb->users); >> cb->skb = skb; >> @@ -1796,19 +1805,27 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, >> return -ECONNREFUSED; >> } >> nlk = nlk_sk(sk); >> - /* A dump is in progress... */ >> + >> mutex_lock(nlk->cb_mutex); >> + /* A dump is in progress... */ >> if (nlk->cb) { >> mutex_unlock(nlk->cb_mutex); >> netlink_destroy_callback(cb); >> - sock_put(sk); >> - return -EBUSY; >> + ret = -EBUSY; >> + goto out; >> + } >> + /* add reference of module witch cb->dump belone to */ >> + if (cb->module && !try_module_get(cb->module)) { >> + mutex_unlock(nlk->cb_mutex); >> + ret = -EPROTONOSUPPORT; >> + goto out; > > Looks like you leak the allocated netlink_callback here. > You should call netlink_destroy_callback() before you exit. > oops, I will fix it,thanks very much! -- 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 Wed, 2012-09-26 at 12:52 +0800, Gao feng wrote: > +int netlink_dump_done(struct netlink_callback *cb) > +{ > + if (cb->module) > + module_put(cb->module); > + return 0; > +} > +EXPORT_SYMBOL(netlink_dump_done); > + No need to test cb->module being not NULL int netlink_dump_done(struct netlink_callback *cb) { module_put(cb->module); return 0; } Same remark for try_module_get() call -- 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
于 2012年09月26日 15:07, Eric Dumazet 写道: > On Wed, 2012-09-26 at 12:52 +0800, Gao feng wrote: > >> +int netlink_dump_done(struct netlink_callback *cb) >> +{ >> + if (cb->module) >> + module_put(cb->module); >> + return 0; >> +} >> +EXPORT_SYMBOL(netlink_dump_done); >> + > > No need to test cb->module being not NULL > > > int netlink_dump_done(struct netlink_callback *cb) > { > module_put(cb->module); > return 0; > } > > Same remark for try_module_get() call > will fix it in v2 patchset. thanks Eric. -- 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/include/linux/netlink.h b/include/linux/netlink.h index f74dd13..a3641e3 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -232,6 +232,8 @@ struct netlink_callback { struct netlink_callback *cb); int (*done)(struct netlink_callback *cb); void *data; + /* the module that dump function belong to */ + struct module *module; u16 family; u16 min_dump_alloc; unsigned int prev_seq, seq; @@ -251,9 +253,11 @@ struct netlink_dump_control { int (*dump)(struct sk_buff *skb, struct netlink_callback *); int (*done)(struct netlink_callback*); void *data; + struct module *module; u16 min_dump_alloc; }; +extern int netlink_dump_done(struct netlink_callback *cb); extern int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, const struct nlmsghdr *nlh, struct netlink_dump_control *control); diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 5270238..011091c 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1769,6 +1769,14 @@ errout_skb: return err; } +int netlink_dump_done(struct netlink_callback *cb) +{ + if (cb->module) + module_put(cb->module); + return 0; +} +EXPORT_SYMBOL(netlink_dump_done); + int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, const struct nlmsghdr *nlh, struct netlink_dump_control *control) @@ -1786,6 +1794,7 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, cb->done = control->done; cb->nlh = nlh; cb->data = control->data; + cb->module = control->module; cb->min_dump_alloc = control->min_dump_alloc; atomic_inc(&skb->users); cb->skb = skb; @@ -1796,19 +1805,27 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, return -ECONNREFUSED; } nlk = nlk_sk(sk); - /* A dump is in progress... */ + mutex_lock(nlk->cb_mutex); + /* A dump is in progress... */ if (nlk->cb) { mutex_unlock(nlk->cb_mutex); netlink_destroy_callback(cb); - sock_put(sk); - return -EBUSY; + ret = -EBUSY; + goto out; + } + /* add reference of module witch cb->dump belone to */ + if (cb->module && !try_module_get(cb->module)) { + mutex_unlock(nlk->cb_mutex); + ret = -EPROTONOSUPPORT; + goto out; } + nlk->cb = cb; mutex_unlock(nlk->cb_mutex); ret = netlink_dump(sk); - +out: sock_put(sk); if (ret)
I get a panic when I use ss -a and rmmod inet_diag at the same time. it's because netlink_dump use inet_diag_dump witch function belongs to module inet_diag. I search the codes and find many modules have the same problem. We need add reference of the module witch the cb->dump belongs to. since CONFIG_NET is bool,so netlink_dump_start in rtnetlink.c and genetlink.c will never trigger this problem. Thanks for all help from Stephen,Jan and Eric. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- include/linux/netlink.h | 4 ++++ net/netlink/af_netlink.c | 25 +++++++++++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-)