On Wed, Sep 26, 2012 at 08:35:53PM +0800, Gao feng wrote:

于 2012年09月26日 17:58, Pablo Neira Ayuso 写道:

quoted

On Wed, Sep 26, 2012 at 05:42:31PM +0800, Gao feng wrote:

quoted

Hi Pablo:

于 2012年09月26日 17:26, Pablo Neira Ayuso 写道:

quoted

On Wed, Sep 26, 2012 at 04:41:21PM +0800, Gao feng wrote:

quoted

use proper netlink_dump_control.done and .module to avoid panic.

Signed-off-by: Gao feng <redacted>
---
 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.

quoted

quoted

quoted

quoted

 	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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html