| Message ID | 1348648888-24943-4-git-send-email-gaofeng@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
? 2012?09?26? 16:41, Gao feng ??: > use proper netlink_dump_control.done and .module to avoid panic. > > Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> > --- > net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > index 9807f32..509a257 100644 > --- a/net/netfilter/nf_conntrack_netlink.c > +++ b/net/netfilter/nf_conntrack_netlink.c > @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) > nf_ct_put((struct nf_conn *)cb->args[1]); > if (cb->data) > kfree(cb->data); > + netlink_dump_done(cb); > return 0; > } > > @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, > struct netlink_dump_control c = { > .dump = ctnetlink_dump_table, > .done = ctnetlink_done, > + .module = THIS_MODULE, > }; > #ifdef CONFIG_NF_CONNTRACK_MARK > if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) { > @@ -1706,6 +1708,8 @@ ctnetlink_stat_ct_cpu(struct sock *ctnl, struct sk_buff *skb, > if (nlh->nlmsg_flags & NLM_F_DUMP) { > struct netlink_dump_control c = { > .dump = ctnetlink_ct_stat_cpu_dump, > + .done = netlink_dump_done, > + .module = THIS_MODULE, > }; > return netlink_dump_start(ctnl, skb, nlh, &c); > } > @@ -2141,6 +2145,7 @@ static int ctnetlink_exp_done(struct netlink_callback *cb) > { > if (cb->args[1]) > nf_ct_expect_put((struct nf_conntrack_expect *)cb->args[1]); > + netlink_dump_done(cb); > return 0; > } I should do return netlink_dump_done here. I will reset this patchset. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: > use proper netlink_dump_control.done and .module to avoid panic. > > Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> > --- > net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > index 9807f32..509a257 100644 > --- a/net/netfilter/nf_conntrack_netlink.c > +++ b/net/netfilter/nf_conntrack_netlink.c > @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) > nf_ct_put((struct nf_conn *)cb->args[1]); > if (cb->data) > kfree(cb->data); > + netlink_dump_done(cb); I think you can call netlink_dump_done from af_netlink.c: static int netlink_dump(struct sock *sk) ... if (cb->done) { cb->done(cb); netlink_dump_done(...); } Thus, you don't need to change netlink_dump_control in every netlink subsystem. > return 0; > } > > @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, > struct netlink_dump_control c = { > .dump = ctnetlink_dump_table, > .done = ctnetlink_done, > + .module = THIS_MODULE, You can do something similar to: 9f00d97 netlink: hide struct module parameter in netlink_kernel_create by definiting netlink_dump_start as static inline and using THIS_MODULE from there. If I'm not missing anything, with those two changes, you will not need to modify any caller and it will result one single patch. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo: ? 2012?09?26? 17:26, Pablo Neira Ayuso ??: > On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: >> use proper netlink_dump_control.done and .module to avoid panic. >> >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> >> --- >> net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c >> index 9807f32..509a257 100644 >> --- a/net/netfilter/nf_conntrack_netlink.c >> +++ b/net/netfilter/nf_conntrack_netlink.c >> @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) >> nf_ct_put((struct nf_conn *)cb->args[1]); >> if (cb->data) >> kfree(cb->data); >> + netlink_dump_done(cb); > > I think you can call netlink_dump_done from af_netlink.c: > > static int netlink_dump(struct sock *sk) > ... > if (cb->done) { > cb->done(cb); > netlink_dump_done(...); > } > > Thus, you don't need to change netlink_dump_control in every netlink > subsystem. because cb->done is called by netlink_sock_destruct too,it's very usefully when userspace program only send dump request to kernel without reading data from kernel. > >> return 0; >> } >> >> @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, >> struct netlink_dump_control c = { >> .dump = ctnetlink_dump_table, >> .done = ctnetlink_done, >> + .module = THIS_MODULE, > > You can do something similar to: > > 9f00d97 netlink: hide struct module parameter in netlink_kernel_create > > by definiting netlink_dump_start as static inline and using > THIS_MODULE from there. > > If I'm not missing anything, with those two changes, you will not need > to modify any caller and it will result one single patch. > You can see the patch [11/11], THIS_MODULE in infiniband/core/cma.c means module rdma_cm,but we call netlink_dump_start in infiniband/core/netlink.c we should make sure the cb.moudle point to the module which cb.dump belongs to. we can call netlink_dump_start to set cb->dump everywhere, so I think we still need to pass struct module to the netlink_callback. thanks for your comments! -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 26, 2012 at 05:42:31PM +0800, Gao feng wrote: > Hi Pablo: > > ? 2012?09?26? 17:26, Pablo Neira Ayuso ??: > > On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: > >> use proper netlink_dump_control.done and .module to avoid panic. > >> > >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> > >> --- > >> net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ > >> 1 files changed, 8 insertions(+), 0 deletions(-) > >> > >> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > >> index 9807f32..509a257 100644 > >> --- a/net/netfilter/nf_conntrack_netlink.c > >> +++ b/net/netfilter/nf_conntrack_netlink.c > >> @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) > >> nf_ct_put((struct nf_conn *)cb->args[1]); > >> if (cb->data) > >> kfree(cb->data); > >> + netlink_dump_done(cb); > > > > I think you can call netlink_dump_done from af_netlink.c: > > > > static int netlink_dump(struct sock *sk) > > ... > > if (cb->done) { > > cb->done(cb); > > netlink_dump_done(...); > > } > > > > Thus, you don't need to change netlink_dump_control in every netlink > > subsystem. > > because cb->done is called by netlink_sock_destruct too,it's very usefully > when userspace program only send dump request to kernel without reading > data from kernel. Then add that to netlink_sock_destruct as well. If possible, I prefer if this remains in the netlink core to avoid leaking module refcount if you forget to call netlink_dump_done. > > > >> return 0; > >> } > >> > >> @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, > >> struct netlink_dump_control c = { > >> .dump = ctnetlink_dump_table, > >> .done = ctnetlink_done, > >> + .module = THIS_MODULE, > > > > You can do something similar to: > > > > 9f00d97 netlink: hide struct module parameter in netlink_kernel_create > > > > by definiting netlink_dump_start as static inline and using > > THIS_MODULE from there. > > > > If I'm not missing anything, with those two changes, you will not need > > to modify any caller and it will result one single patch. > > > > You can see the patch [11/11], THIS_MODULE in infiniband/core/cma.c > means module rdma_cm,but we call netlink_dump_start in infiniband/core/netlink.c You can still use __netlink_dump_start for that case, which allows you to specify a custom struct module * parameter. But for most cases, netlink_dump_start (which hides THIS_MODULE) should be fine. > we should make sure the cb.moudle point to the module which cb.dump belongs to. > we can call netlink_dump_start to set cb->dump everywhere, so I think we still > need to pass struct module to the netlink_callback. > > thanks for your comments! > -- > 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 linux-rdma" 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? 17:58, Pablo Neira Ayuso ??: > On Wed, Sep 26, 2012 at 05:42:31PM +0800, Gao feng wrote: >> Hi Pablo: >> >> ? 2012?09?26? 17:26, Pablo Neira Ayuso ??: >>> On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: >>>> use proper netlink_dump_control.done and .module to avoid panic. >>>> >>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> >>>> --- >>>> net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ >>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c >>>> index 9807f32..509a257 100644 >>>> --- a/net/netfilter/nf_conntrack_netlink.c >>>> +++ b/net/netfilter/nf_conntrack_netlink.c >>>> @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) >>>> nf_ct_put((struct nf_conn *)cb->args[1]); >>>> if (cb->data) >>>> kfree(cb->data); >>>> + netlink_dump_done(cb); >>> >>> I think you can call netlink_dump_done from af_netlink.c: >>> >>> static int netlink_dump(struct sock *sk) >>> ... >>> if (cb->done) { >>> cb->done(cb); >>> netlink_dump_done(...); >>> } >>> >>> Thus, you don't need to change netlink_dump_control in every netlink >>> subsystem. >> >> because cb->done is called by netlink_sock_destruct too,it's very usefully >> when userspace program only send dump request to kernel without reading >> data from kernel. > > Then add that to netlink_sock_destruct as well. If possible, I prefer > if this remains in the netlink core to avoid leaking module refcount > if you forget to call netlink_dump_done. make sense,I will update it in next version. Thanks! > >>> >>>> return 0; >>>> } >>>> >>>> @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, >>>> struct netlink_dump_control c = { >>>> .dump = ctnetlink_dump_table, >>>> .done = ctnetlink_done, >>>> + .module = THIS_MODULE, >>> >>> You can do something similar to: >>> >>> 9f00d97 netlink: hide struct module parameter in netlink_kernel_create >>> >>> by definiting netlink_dump_start as static inline and using >>> THIS_MODULE from there. >>> >>> If I'm not missing anything, with those two changes, you will not need >>> to modify any caller and it will result one single patch. >>> >> >> You can see the patch [11/11], THIS_MODULE in infiniband/core/cma.c >> means module rdma_cm,but we call netlink_dump_start in infiniband/core/netlink.c > > You can still use __netlink_dump_start for that case, which allows you > to specify a custom struct module * parameter. But for most cases, > netlink_dump_start (which hides THIS_MODULE) should be fine. > I don't know how to deal with module_put in this way. and I think my way is simple enough. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 26, 2012 at 08:35:53PM +0800, Gao feng wrote: > ? 2012?09?26? 17:58, Pablo Neira Ayuso ??: > > On Wed, Sep 26, 2012 at 05:42:31PM +0800, Gao feng wrote: > >> Hi Pablo: > >> > >> ? 2012?09?26? 17:26, Pablo Neira Ayuso ??: > >>> On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: > >>>> use proper netlink_dump_control.done and .module to avoid panic. > >>>> > >>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> > >>>> --- > >>>> net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ > >>>> 1 files changed, 8 insertions(+), 0 deletions(-) > >>>> > >>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > >>>> index 9807f32..509a257 100644 > >>>> --- a/net/netfilter/nf_conntrack_netlink.c > >>>> +++ b/net/netfilter/nf_conntrack_netlink.c > >>>> @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) > >>>> nf_ct_put((struct nf_conn *)cb->args[1]); > >>>> if (cb->data) > >>>> kfree(cb->data); > >>>> + netlink_dump_done(cb); > >>> > >>> I think you can call netlink_dump_done from af_netlink.c: > >>> > >>> static int netlink_dump(struct sock *sk) > >>> ... > >>> if (cb->done) { > >>> cb->done(cb); > >>> netlink_dump_done(...); > >>> } > >>> > >>> Thus, you don't need to change netlink_dump_control in every netlink > >>> subsystem. > >> > >> because cb->done is called by netlink_sock_destruct too,it's very usefully > >> when userspace program only send dump request to kernel without reading > >> data from kernel. > > > > Then add that to netlink_sock_destruct as well. If possible, I prefer > > if this remains in the netlink core to avoid leaking module refcount > > if you forget to call netlink_dump_done. > > make sense, I will update it in next version. > Thanks! Great. Remove also netlink_dump_done and just use module_put instead after cb->done. That new function you added is so small that there is no way to justify its addition. > > > >>> > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, > >>>> struct netlink_dump_control c = { > >>>> .dump = ctnetlink_dump_table, > >>>> .done = ctnetlink_done, > >>>> + .module = THIS_MODULE, > >>> > >>> You can do something similar to: > >>> > >>> 9f00d97 netlink: hide struct module parameter in netlink_kernel_create > >>> > >>> by definiting netlink_dump_start as static inline and using > >>> THIS_MODULE from there. > >>> > >>> If I'm not missing anything, with those two changes, you will not need > >>> to modify any caller and it will result one single patch. > >>> > >> > >> You can see the patch [11/11], THIS_MODULE in infiniband/core/cma.c > >> means module rdma_cm,but we call netlink_dump_start in infiniband/core/netlink.c > > > > You can still use __netlink_dump_start for that case, which allows you > > to specify a custom struct module * parameter. But for most cases, > > netlink_dump_start (which hides THIS_MODULE) should be fine. > > I don't know how to deal with module_put in this way. > and I think my way is simple enough. Not sure what problem with module_put you're refering to. I think you can make this patchset way smaller with the change I'm proposing. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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? 23:04, Pablo Neira Ayuso ??: > On Wed, Sep 26, 2012 at 08:35:53PM +0800, Gao feng wrote: >> ? 2012?09?26? 17:58, Pablo Neira Ayuso ??: >>> On Wed, Sep 26, 2012 at 05:42:31PM +0800, Gao feng wrote: >>>> Hi Pablo: >>>> >>>> ? 2012?09?26? 17:26, Pablo Neira Ayuso ??: >>>>> On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote: >>>>>> use proper netlink_dump_control.done and .module to avoid panic. >>>>>> >>>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> >>>>>> --- >>>>>> net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ >>>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>>> >>>>>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c >>>>>> index 9807f32..509a257 100644 >>>>>> --- a/net/netfilter/nf_conntrack_netlink.c >>>>>> +++ b/net/netfilter/nf_conntrack_netlink.c >>>>>> @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) >>>>>> nf_ct_put((struct nf_conn *)cb->args[1]); >>>>>> if (cb->data) >>>>>> kfree(cb->data); >>>>>> + netlink_dump_done(cb); >>>>> >>>>> I think you can call netlink_dump_done from af_netlink.c: >>>>> >>>>> static int netlink_dump(struct sock *sk) >>>>> ... >>>>> if (cb->done) { >>>>> cb->done(cb); >>>>> netlink_dump_done(...); >>>>> } >>>>> >>>>> Thus, you don't need to change netlink_dump_control in every netlink >>>>> subsystem. >>>> >>>> because cb->done is called by netlink_sock_destruct too,it's very usefully >>>> when userspace program only send dump request to kernel without reading >>>> data from kernel. >>> >>> Then add that to netlink_sock_destruct as well. If possible, I prefer >>> if this remains in the netlink core to avoid leaking module refcount >>> if you forget to call netlink_dump_done. >> >> make sense, I will update it in next version. >> Thanks! > > Great. Remove also netlink_dump_done and just use module_put instead > after cb->done. That new function you added is so small that there is > no way to justify its addition. > >>> >>>>> >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, >>>>>> struct netlink_dump_control c = { >>>>>> .dump = ctnetlink_dump_table, >>>>>> .done = ctnetlink_done, >>>>>> + .module = THIS_MODULE, >>>>> >>>>> You can do something similar to: >>>>> >>>>> 9f00d97 netlink: hide struct module parameter in netlink_kernel_create >>>>> >>>>> by definiting netlink_dump_start as static inline and using >>>>> THIS_MODULE from there. >>>>> >>>>> If I'm not missing anything, with those two changes, you will not need >>>>> to modify any caller and it will result one single patch. >>>>> >>>> >>>> You can see the patch [11/11], THIS_MODULE in infiniband/core/cma.c >>>> means module rdma_cm,but we call netlink_dump_start in infiniband/core/netlink.c >>> >>> You can still use __netlink_dump_start for that case, which allows you >>> to specify a custom struct module * parameter. But for most cases, >>> netlink_dump_start (which hides THIS_MODULE) should be fine. >> >> I don't know how to deal with module_put in this way. >> and I think my way is simple enough. > > Not sure what problem with module_put you're refering to. > forget this,I'm wrong. > I think you can make this patchset way smaller with the change I'm > proposing. > I know,but I choose simple and directviewing, not smaller. I think these two function will make people confuse. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index 9807f32..509a257 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb) nf_ct_put((struct nf_conn *)cb->args[1]); if (cb->data) kfree(cb->data); + netlink_dump_done(cb); return 0; } @@ -1022,6 +1023,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb, struct netlink_dump_control c = { .dump = ctnetlink_dump_table, .done = ctnetlink_done, + .module = THIS_MODULE, }; #ifdef CONFIG_NF_CONNTRACK_MARK if (cda[CTA_MARK] && cda[CTA_MARK_MASK]) { @@ -1706,6 +1708,8 @@ ctnetlink_stat_ct_cpu(struct sock *ctnl, struct sk_buff *skb, if (nlh->nlmsg_flags & NLM_F_DUMP) { struct netlink_dump_control c = { .dump = ctnetlink_ct_stat_cpu_dump, + .done = netlink_dump_done, + .module = THIS_MODULE, }; return netlink_dump_start(ctnl, skb, nlh, &c); } @@ -2141,6 +2145,7 @@ static int ctnetlink_exp_done(struct netlink_callback *cb) { if (cb->args[1]) nf_ct_expect_put((struct nf_conntrack_expect *)cb->args[1]); + netlink_dump_done(cb); return 0; } @@ -2222,6 +2227,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb, struct netlink_dump_control c = { .dump = ctnetlink_exp_dump_table, .done = ctnetlink_exp_done, + .module = THIS_MODULE, }; return netlink_dump_start(ctnl, skb, nlh, &c); } @@ -2660,6 +2666,8 @@ ctnetlink_stat_exp_cpu(struct sock *ctnl, struct sk_buff *skb, if (nlh->nlmsg_flags & NLM_F_DUMP) { struct netlink_dump_control c = { .dump = ctnetlink_exp_stat_cpu_dump, + .done = netlink_dump_done, + .module = THIS_MODULE, }; return netlink_dump_start(ctnl, skb, nlh, &c); }
use proper netlink_dump_control.done and .module to avoid panic. Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com> --- net/netfilter/nf_conntrack_netlink.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)