Message ID | 20180904021241.11426-1-xiyou.wangcong@gmail.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net] tipc: call start and done ops directly in __tipc_nl_compat_dumpit() | expand |
On 09/04/2018 10:12 AM, Cong Wang wrote: > __tipc_nl_compat_dumpit() uses a netlink_callback on stack, > so the only way to align it with other ->dumpit() call path > is calling tipc_dump_start() and tipc_dump_done() directly > inside it. Otherwise ->dumpit() would always get NULL from > cb->args[0]. Thank you for your fix Cong! Your solution is fine with me. When we align __tipc_nl_compat_dumpit() with ->dumpit() functions defined in tipc_genl_v2_ops[], cb->args[0] is used to save a rhashtable_iter object allocated in tipc_dump_start(), and the object will be retrieved with cb->args[0] in tipc_dump_done() and will be freed. But unfortunately cb->args[0] has been used to other purposes in tipc_nl_bearer_dump(), tipc_nl_node_dump_link(), tipc_nl_name_table_dump(), tipc_nl_node_dump() and tipc_nl_net_dump(). It means cb->args[0] saved in __tipc_dump_start() will be overwritten in these ->dumpit() functions. As a consequence, not only the rhashtable_iter object allocated in tipc_dump_start() cannot be properly released in tipc_dump_done(), but also more kernel panics might be triggered in tipc_dump_done(). > > But, tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve > net pointer, the cb->skb here doesn't set ->sk, the net pointer > is saved in msg->net instead, so introduce a helper function > __tipc_dump_start() to pass in msg->net. > > Fixes: 9a07efa9aea2 ("tipc: switch to rhashtable iterator") > Reported-by: syzbot+e93a2c41f91b8e2c7d9b@syzkaller.appspotmail.com > Cc: Jon Maloy <jon.maloy@ericsson.com> > Cc: Ying Xue <ying.xue@windriver.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/tipc/netlink_compat.c | 2 ++ > net/tipc/socket.c | 8 ++++++-- > net/tipc/socket.h | 1 + > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c > index a2f76743c73a..82f665728382 100644 > --- a/net/tipc/netlink_compat.c > +++ b/net/tipc/netlink_compat.c > @@ -185,6 +185,7 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd, > return -ENOMEM; > > buf->sk = msg->dst_sk; > + __tipc_dump_start(&cb, msg->net); > > do { > int rem; > @@ -216,6 +217,7 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd, > err = 0; > > err_out: > + tipc_dump_done(&cb); > kfree_skb(buf); > > if (err == -EMSGSIZE) { > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index ab7a2a7178f7..a19b2b1c77ed 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -3264,9 +3264,14 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb, > EXPORT_SYMBOL(tipc_nl_sk_walk); > > int tipc_dump_start(struct netlink_callback *cb) > +{ > + return __tipc_dump_start(cb, sock_net(cb->skb->sk)); > +} > +EXPORT_SYMBOL(tipc_dump_start); > + > +int __tipc_dump_start(struct netlink_callback *cb, struct net *net) > { > struct rhashtable_iter *iter = (void *)cb->args[0]; > - struct net *net = sock_net(cb->skb->sk); > struct tipc_net *tn = tipc_net(net); > > if (!iter) { > @@ -3280,7 +3285,6 @@ int tipc_dump_start(struct netlink_callback *cb) > rhashtable_walk_enter(&tn->sk_rht, iter); > return 0; > } > -EXPORT_SYMBOL(tipc_dump_start); > > int tipc_dump_done(struct netlink_callback *cb) > { > diff --git a/net/tipc/socket.h b/net/tipc/socket.h > index d43032e26532..5e575f205afe 100644 > --- a/net/tipc/socket.h > +++ b/net/tipc/socket.h > @@ -69,5 +69,6 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb, > struct netlink_callback *cb, > struct tipc_sock *tsk)); > int tipc_dump_start(struct netlink_callback *cb); > +int __tipc_dump_start(struct netlink_callback *cb, struct net *net); > int tipc_dump_done(struct netlink_callback *cb); > #endif >
On Tue, Sep 4, 2018 at 4:45 AM Ying Xue <ying.xue@windriver.com> wrote: > > > On 09/04/2018 10:12 AM, Cong Wang wrote: > > __tipc_nl_compat_dumpit() uses a netlink_callback on stack, > > so the only way to align it with other ->dumpit() call path > > is calling tipc_dump_start() and tipc_dump_done() directly > > inside it. Otherwise ->dumpit() would always get NULL from > > cb->args[0]. > > Thank you for your fix Cong! > > Your solution is fine with me. > > When we align __tipc_nl_compat_dumpit() with ->dumpit() functions > defined in tipc_genl_v2_ops[], cb->args[0] is used to save a > rhashtable_iter object allocated in tipc_dump_start(), and the object > will be retrieved with cb->args[0] in tipc_dump_done() and will be freed. > > But unfortunately cb->args[0] has been used to other purposes in > tipc_nl_bearer_dump(), tipc_nl_node_dump_link(), > tipc_nl_name_table_dump(), tipc_nl_node_dump() and tipc_nl_net_dump(). > It means cb->args[0] saved in __tipc_dump_start() will be overwritten in > these ->dumpit() functions. As a consequence, not only the > rhashtable_iter object allocated in tipc_dump_start() cannot be properly > released in tipc_dump_done(), but also more kernel panics might be > triggered in tipc_dump_done(). Ah, good catch! The max utilization of cb->args is tipc_nl_name_table_dump(): net/tipc/name_table.c: cb->args[0] = last_type; net/tipc/name_table.c: cb->args[1] = last_lower; net/tipc/name_table.c: cb->args[2] = last_key; net/tipc/name_table.c: cb->args[3] = done; Looks like I should just use cb->args[4] for rhashtable iterator, as we still have some room in cb->args[].
diff --git a/net/tipc/netlink_compat.c b/net/tipc/netlink_compat.c index a2f76743c73a..82f665728382 100644 --- a/net/tipc/netlink_compat.c +++ b/net/tipc/netlink_compat.c @@ -185,6 +185,7 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd, return -ENOMEM; buf->sk = msg->dst_sk; + __tipc_dump_start(&cb, msg->net); do { int rem; @@ -216,6 +217,7 @@ static int __tipc_nl_compat_dumpit(struct tipc_nl_compat_cmd_dump *cmd, err = 0; err_out: + tipc_dump_done(&cb); kfree_skb(buf); if (err == -EMSGSIZE) { diff --git a/net/tipc/socket.c b/net/tipc/socket.c index ab7a2a7178f7..a19b2b1c77ed 100644 --- a/net/tipc/socket.c +++ b/net/tipc/socket.c @@ -3264,9 +3264,14 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb, EXPORT_SYMBOL(tipc_nl_sk_walk); int tipc_dump_start(struct netlink_callback *cb) +{ + return __tipc_dump_start(cb, sock_net(cb->skb->sk)); +} +EXPORT_SYMBOL(tipc_dump_start); + +int __tipc_dump_start(struct netlink_callback *cb, struct net *net) { struct rhashtable_iter *iter = (void *)cb->args[0]; - struct net *net = sock_net(cb->skb->sk); struct tipc_net *tn = tipc_net(net); if (!iter) { @@ -3280,7 +3285,6 @@ int tipc_dump_start(struct netlink_callback *cb) rhashtable_walk_enter(&tn->sk_rht, iter); return 0; } -EXPORT_SYMBOL(tipc_dump_start); int tipc_dump_done(struct netlink_callback *cb) { diff --git a/net/tipc/socket.h b/net/tipc/socket.h index d43032e26532..5e575f205afe 100644 --- a/net/tipc/socket.h +++ b/net/tipc/socket.h @@ -69,5 +69,6 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb, struct netlink_callback *cb, struct tipc_sock *tsk)); int tipc_dump_start(struct netlink_callback *cb); +int __tipc_dump_start(struct netlink_callback *cb, struct net *net); int tipc_dump_done(struct netlink_callback *cb); #endif
__tipc_nl_compat_dumpit() uses a netlink_callback on stack, so the only way to align it with other ->dumpit() call path is calling tipc_dump_start() and tipc_dump_done() directly inside it. Otherwise ->dumpit() would always get NULL from cb->args[0]. But, tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve net pointer, the cb->skb here doesn't set ->sk, the net pointer is saved in msg->net instead, so introduce a helper function __tipc_dump_start() to pass in msg->net. Fixes: 9a07efa9aea2 ("tipc: switch to rhashtable iterator") Reported-by: syzbot+e93a2c41f91b8e2c7d9b@syzkaller.appspotmail.com Cc: Jon Maloy <jon.maloy@ericsson.com> Cc: Ying Xue <ying.xue@windriver.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/tipc/netlink_compat.c | 2 ++ net/tipc/socket.c | 8 ++++++-- net/tipc/socket.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)