| Message ID | 20210708092936.20044-1-yajun.deng@linux.dev (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: rtnetlink: Fix rtnl_dereference may be return NULL | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: fw@strlen.de; 1 maintainers not CCed: fw@strlen.de |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3399 this patch: 3399 |
| netdev/kdoc | success | Errors and warnings before: 7 this patch: 7 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 44 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3456 this patch: 3456 |
| netdev/header_inline | success | Link |
On Thu, 2021-07-08 at 17:29 +0800, Yajun Deng wrote: > The value 'link' may be NULL in rtnl_unregister(), this leads to > kfree_rcu(NULL, xxx), so add this case handling. > I don't see how. It would require the caller to unregister something they never registered. That would be a bug there, but I don't see that it's very useful to actually be defensive about bugs there. > And modify the return > value to 'void' in rtnl_unregister(). there is no case using it. > > Fixes: addf9b90de22 (net: rtnetlink: use rcu to free rtnl message handlers) > Fixes: 51e13685bd93 (rtnetlink: RCU-annotate both dimensions of rtnl_msg_handlers) It certainly fixes nothing in those patches. johannes
On Thu, Jul 08, 2021 at 11:43:20AM +0200, Johannes Berg wrote: > On Thu, 2021-07-08 at 17:29 +0800, Yajun Deng wrote: > > The value 'link' may be NULL in rtnl_unregister(), this leads to > > kfree_rcu(NULL, xxx), so add this case handling. > > > > I don't see how. It would require the caller to unregister something > they never registered. That would be a bug there, but I don't see that > it's very useful to actually be defensive about bugs there. Besides, isn't kfree_rcu(NULL) safe anyway?
On 7/8/21 1:11 PM, Vladimir Oltean wrote: > On Thu, Jul 08, 2021 at 11:43:20AM +0200, Johannes Berg wrote: >> On Thu, 2021-07-08 at 17:29 +0800, Yajun Deng wrote: >>> The value 'link' may be NULL in rtnl_unregister(), this leads to >>> kfree_rcu(NULL, xxx), so add this case handling. >>> >> >> I don't see how. It would require the caller to unregister something >> they never registered. That would be a bug there, but I don't see that >> it's very useful to actually be defensive about bugs there. > > Besides, isn't kfree_rcu(NULL) safe anyway? > Only from linux-5.3 I think. (commit 12edff045bc6dd3ab1565cc02fa4841803c2a633 was not backported to old kernels) But yes, this patch is not solving any bug, as I suspected.
diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h index 384e800665f2..9d263ad9ea48 100644 --- a/include/net/rtnetlink.h +++ b/include/net/rtnetlink.h @@ -17,7 +17,7 @@ void rtnl_register(int protocol, int msgtype, rtnl_doit_func, rtnl_dumpit_func, unsigned int flags); int rtnl_register_module(struct module *owner, int protocol, int msgtype, rtnl_doit_func, rtnl_dumpit_func, unsigned int flags); -int rtnl_unregister(int protocol, int msgtype); +void rtnl_unregister(int protocol, int msgtype); void rtnl_unregister_all(int protocol); static inline int rtnl_msg_family(const struct nlmsghdr *nlh) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index f6af3e74fc44..e80177c195a5 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -281,10 +281,8 @@ void rtnl_register(int protocol, int msgtype, * rtnl_unregister - Unregister a rtnetlink message type * @protocol: Protocol family or PF_UNSPEC * @msgtype: rtnetlink message type - * - * Returns 0 on success or a negative error code. */ -int rtnl_unregister(int protocol, int msgtype) +void rtnl_unregister(int protocol, int msgtype) { struct rtnl_link __rcu **tab; struct rtnl_link *link; @@ -295,18 +293,18 @@ int rtnl_unregister(int protocol, int msgtype) rtnl_lock(); tab = rtnl_dereference(rtnl_msg_handlers[protocol]); - if (!tab) { - rtnl_unlock(); - return -ENOENT; - } + if (!tab) + goto unlock; link = rtnl_dereference(tab[msgindex]); - rcu_assign_pointer(tab[msgindex], NULL); - rtnl_unlock(); + if (!link) + goto unlock; + rcu_assign_pointer(tab[msgindex], NULL); kfree_rcu(link, rcu); - return 0; +unlock: + rtnl_unlock(); } EXPORT_SYMBOL_GPL(rtnl_unregister);
The value 'link' may be NULL in rtnl_unregister(), this leads to kfree_rcu(NULL, xxx), so add this case handling. And modify the return value to 'void' in rtnl_unregister(). there is no case using it. Fixes: addf9b90de22 (net: rtnetlink: use rcu to free rtnl message handlers) Fixes: 51e13685bd93 (rtnetlink: RCU-annotate both dimensions of rtnl_msg_handlers) Signed-off-by: Yajun Deng <yajun.deng@linux.dev> --- include/net/rtnetlink.h | 2 +- net/core/rtnetlink.c | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-)