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.
Thanks for all help from Stephen,Jan,Eric and Steffen.
Signed-off-by: Gao feng <redacted>
---
include/linux/netlink.h | 6 +++++-
net/netlink/af_netlink.c | 25 +++++++++++++++++++++----
2 files changed, 26 insertions(+), 5 deletions(-)
@@ -232,6 +232,8 @@ struct netlink_callback {structnetlink_callback*cb);int(*done)(structnetlink_callback*cb);void*data;+/* the module that dump function belong to */+structmodule*module;u16family;u16min_dump_alloc;unsignedintprev_seq,seq;
@@ -249,11 +251,13 @@ __nlmsg_put(struct sk_buff *skb, u32 pid, u32 seq, int type, int len, int flags)structnetlink_dump_control{int(*dump)(structsk_buff*skb,structnetlink_callback*);-int(*done)(structnetlink_callback*);+int(*done)(structnetlink_callback*);void*data;+structmodule*module;u16min_dump_alloc;};+externintnetlink_dump_done(structnetlink_callback*cb);externintnetlink_dump_start(structsock*ssk,structsk_buff*skb,conststructnlmsghdr*nlh,structnetlink_dump_control*control);
@@ -1796,19 +1804,28 @@ 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;+gotoout;+}+/* add reference of module witch cb->dump belong to */+if(!try_module_get(cb->module)){+mutex_unlock(nlk->cb_mutex);+netlink_destroy_callback(cb);+ret=-EPROTONOSUPPORT;+gotoout;}+nlk->cb=cb;mutex_unlock(nlk->cb_mutex);ret=netlink_dump(sk);-+out:sock_put(sk);if(ret)
@@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb)nf_ct_put((structnf_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.
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.
@@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb)nf_ct_put((structnf_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.
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!
@@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb)nf_ct_put((structnf_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.
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -706,6 +706,7 @@ static int ctnetlink_done(struct netlink_callback *cb)nf_ct_put((structnf_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!
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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html