[Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[Cong Wang]

__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[].

But tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve
net pointer, the cb->skb here doesn't set skb->sk, the net pointer
is saved in msg->net instead, so introduce a helper function
__tipc_dump_start() to pass in msg->net.

Ying pointed out cb->args[0...3] are already used by other
callbacks on this call path, so we can't use cb->args[0] any
more, use cb->args[4] instead.

Fixes: 9a07efa9aea2 ("tipc: switch to rhashtable iterator")
Reported-and-tested-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>
---

v2: fix sock_net(cb->skb->sk)
v3: use cb->args[4] instead of cb->args[0]

 net/tipc/netlink_compat.c |  2 ++
 net/tipc/socket.c         | 17 +++++++++++------
 net/tipc/socket.h         |  1 +
 3 files changed, 14 insertions(+), 6 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 a0ff8bffc96b..3f03ddd0e35b 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -3230,7 +3230,7 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb,
 				       struct netlink_callback *cb,
 				       struct tipc_sock *tsk))
 {
-	struct rhashtable_iter *iter = (void *)cb->args[0];
+	struct rhashtable_iter *iter = (void *)cb->args[4];
 	struct tipc_sock *tsk;
 	int err;
 
@@ -3266,8 +3266,14 @@ EXPORT_SYMBOL(tipc_nl_sk_walk);
 
 int tipc_dump_start(struct netlink_callback *cb)
 {
-	struct rhashtable_iter *iter = (void *)cb->args[0];
-	struct net *net = sock_net(cb->skb->sk);
+	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)
+{
+	/* tipc_nl_name_table_dump() uses cb->args[0...3]. */
+	struct rhashtable_iter *iter = (void *)cb->args[4];
 	struct tipc_net *tn = tipc_net(net);
 
 	if (!iter) {
@@ -3275,17 +3281,16 @@ int tipc_dump_start(struct netlink_callback *cb)
 		if (!iter)
 			return -ENOMEM;
 
-		cb->args[0] = (long)iter;
+		cb->args[4] = (long)iter;
 	}
 
 	rhashtable_walk_enter(&tn->sk_rht, iter);
 	return 0;
 }
-EXPORT_SYMBOL(tipc_dump_start);
 
 int tipc_dump_done(struct netlink_callback *cb)
 {
-	struct rhashtable_iter *hti = (void *)cb->args[0];
+	struct rhashtable_iter *hti = (void *)cb->args[4];
 
 	rhashtable_walk_exit(hti);
 	kfree(hti);
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
-- 
2.14.4

Re: [Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[Ying Xue]

On 09/05/2018 05:54 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[].
> 
> But tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve
> net pointer, the cb->skb here doesn't set skb->sk, the net pointer
> is saved in msg->net instead, so introduce a helper function
> __tipc_dump_start() to pass in msg->net.
> 
> Ying pointed out cb->args[0...3] are already used by other
> callbacks on this call path, so we can't use cb->args[0] any
> more, use cb->args[4] instead.

It's a common mechanism to save rhashtable iterator pointer in cb->args
after tipc_dump_start() and tipc_dump_done() are introduced. Someday
probably we will involve new dumpit function. In order to lower the risk
that rhashtable iterator pointer saved is overwritten, it's better to
use the last slot, ie, cb->args[5].

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[Cong Wang]

On Tue, Sep 4, 2018 at 8:25 PM Ying Xue <ying.xue@windriver.com> wrote:
>
> On 09/05/2018 05:54 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[].
> >
> > But tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve
> > net pointer, the cb->skb here doesn't set skb->sk, the net pointer
> > is saved in msg->net instead, so introduce a helper function
> > __tipc_dump_start() to pass in msg->net.
> >
> > Ying pointed out cb->args[0...3] are already used by other
> > callbacks on this call path, so we can't use cb->args[0] any
> > more, use cb->args[4] instead.
>
> It's a common mechanism to save rhashtable iterator pointer in cb->args
> after tipc_dump_start() and tipc_dump_done() are introduced. Someday
> probably we will involve new dumpit function. In order to lower the risk
> that rhashtable iterator pointer saved is overwritten, it's better to
> use the last slot, ie, cb->args[5].

I don't understand, currently only cb->args[0..3] are used at most,
therefore cb->args[4] is pretty safe in current code base, there is
no reason to be so defensive to use cb->args[5].

If you really worry about future, you probably want to extend cb->args
from 6 to whatever larger, rather than just skipping cb->args[4].

I don't see any reason to do so.

Re: [Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[Ying Xue]

On 09/05/2018 01:49 PM, Cong Wang wrote:
> On Tue, Sep 4, 2018 at 8:25 PM Ying Xue <ying.xue@windriver.com> wrote:
>>
>> On 09/05/2018 05:54 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[].
>>>
>>> But tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve
>>> net pointer, the cb->skb here doesn't set skb->sk, the net pointer
>>> is saved in msg->net instead, so introduce a helper function
>>> __tipc_dump_start() to pass in msg->net.
>>>
>>> Ying pointed out cb->args[0...3] are already used by other
>>> callbacks on this call path, so we can't use cb->args[0] any
>>> more, use cb->args[4] instead.
>>
>> It's a common mechanism to save rhashtable iterator pointer in cb->args
>> after tipc_dump_start() and tipc_dump_done() are introduced. Someday
>> probably we will involve new dumpit function. In order to lower the risk
>> that rhashtable iterator pointer saved is overwritten, it's better to
>> use the last slot, ie, cb->args[5].
> 
> I don't understand, currently only cb->args[0..3] are used at most,
> therefore cb->args[4] is pretty safe in current code base, there is
> no reason to be so defensive to use cb->args[5].
> 

Yes, at present cb->args[4] is safe.

> If you really worry about future, you probably want to extend cb->args
> from 6 to whatever larger, rather than just skipping cb->args[4].
> 

When we have to use the fifth slot of cb->args[] in the future, we need
to skip sb->args[4], which is a bit wried. This is the reason why I
suggested we could use the last one.

> I don't see any reason to do so.

As I said, the current version is safe. If you think it's unnecessary to
change, it's okay to me.

Acked-by: Ying Xue <ying.xue@windriver.com>

> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [Patch net v3] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue,  4 Sep 2018 14:54:55 -0700

> __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[].
> 
> But tipc_dump_start() uses sock_net(cb->skb->sk) to retrieve
> net pointer, the cb->skb here doesn't set skb->sk, the net pointer
> is saved in msg->net instead, so introduce a helper function
> __tipc_dump_start() to pass in msg->net.
> 
> Ying pointed out cb->args[0...3] are already used by other
> callbacks on this call path, so we can't use cb->args[0] any
> more, use cb->args[4] instead.
> 
> Fixes: 9a07efa9aea2 ("tipc: switch to rhashtable iterator")
> Reported-and-tested-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>

Applied, thanks Cong.