* [PATCH net] netlink: access nlk groups safely in netlink bind and getname
@ 2017-09-06 3:53 Xin Long
2017-09-07 4:24 ` David Miller
2017-09-08 19:35 ` Cong Wang
0 siblings, 2 replies; 5+ messages in thread
From: Xin Long @ 2017-09-06 3:53 UTC (permalink / raw)
To: network dev; +Cc: davem, fw
Now there is no lock protecting nlk ngroups/groups' accessing in
netlink bind and getname. It's safe from nlk groups' setting in
netlink_release, but not from netlink_realloc_groups called by
netlink_setsockopt.
netlink_lock_table is needed in both netlink bind and getname when
accessing nlk groups.
Acked-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/netlink/af_netlink.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 94a61e6..3278077 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -955,7 +955,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
struct net *net = sock_net(sk);
struct netlink_sock *nlk = nlk_sk(sk);
struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
- int err;
+ int err = 0;
long unsigned int groups = nladdr->nl_groups;
bool bound;
@@ -983,6 +983,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
return -EINVAL;
}
+ netlink_lock_table();
if (nlk->netlink_bind && groups) {
int group;
@@ -993,7 +994,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
if (!err)
continue;
netlink_undo_bind(group, groups, sk);
- return err;
+ goto unlock;
}
}
@@ -1006,12 +1007,13 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
netlink_autobind(sock);
if (err) {
netlink_undo_bind(nlk->ngroups, groups, sk);
- return err;
+ goto unlock;
}
}
if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
- return 0;
+ goto unlock;
+ netlink_unlock_table();
netlink_table_grab();
netlink_update_subscriptions(sk, nlk->subscriptions +
@@ -1022,6 +1024,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
netlink_table_ungrab();
return 0;
+
+unlock:
+ netlink_unlock_table();
+ return err;
}
static int netlink_connect(struct socket *sock, struct sockaddr *addr,
@@ -1079,7 +1085,9 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr,
nladdr->nl_groups = netlink_group_mask(nlk->dst_group);
} else {
nladdr->nl_pid = nlk->portid;
+ netlink_lock_table();
nladdr->nl_groups = nlk->groups ? nlk->groups[0] : 0;
+ netlink_unlock_table();
}
return 0;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname
2017-09-06 3:53 [PATCH net] netlink: access nlk groups safely in netlink bind and getname Xin Long
@ 2017-09-07 4:24 ` David Miller
2017-09-08 19:35 ` Cong Wang
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-09-07 4:24 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, fw
From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 6 Sep 2017 11:53:29 +0800
> Now there is no lock protecting nlk ngroups/groups' accessing in
> netlink bind and getname. It's safe from nlk groups' setting in
> netlink_release, but not from netlink_realloc_groups called by
> netlink_setsockopt.
>
> netlink_lock_table is needed in both netlink bind and getname when
> accessing nlk groups.
>
> Acked-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname
2017-09-06 3:53 [PATCH net] netlink: access nlk groups safely in netlink bind and getname Xin Long
2017-09-07 4:24 ` David Miller
@ 2017-09-08 19:35 ` Cong Wang
2017-09-10 11:45 ` Xin Long
1 sibling, 1 reply; 5+ messages in thread
From: Cong Wang @ 2017-09-08 19:35 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, David Miller, Florian Westphal
On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
> Now there is no lock protecting nlk ngroups/groups' accessing in
> netlink bind and getname. It's safe from nlk groups' setting in
> netlink_release, but not from netlink_realloc_groups called by
> netlink_setsockopt.
>
> netlink_lock_table is needed in both netlink bind and getname when
> accessing nlk groups.
This looks very odd.
netlink_lock_table() should be protecting nl_table, why
it also protects nlk->groups?? For me it looks like you
need lock_sock() instead.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname
2017-09-08 19:35 ` Cong Wang
@ 2017-09-10 11:45 ` Xin Long
2017-09-11 17:35 ` Cong Wang
0 siblings, 1 reply; 5+ messages in thread
From: Xin Long @ 2017-09-10 11:45 UTC (permalink / raw)
To: Cong Wang; +Cc: network dev, David Miller, Florian Westphal
On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
>> Now there is no lock protecting nlk ngroups/groups' accessing in
>> netlink bind and getname. It's safe from nlk groups' setting in
>> netlink_release, but not from netlink_realloc_groups called by
>> netlink_setsockopt.
>>
>> netlink_lock_table is needed in both netlink bind and getname when
>> accessing nlk groups.
>
> This looks very odd.
>
> netlink_lock_table() should be protecting nl_table, why
> it also protects nlk->groups?? For me it looks like you
> need lock_sock() instead.
I believe netlink_lock_table might be only used to protect nl_table
at the beginning and surely lock_sock is better here. Thanks.
But can you explain why netlink_lock_table() was also used in
netlink_getsockopt NETLINK_LIST_MEMBERSHIPS ? or it
was just a mistake ?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname
2017-09-10 11:45 ` Xin Long
@ 2017-09-11 17:35 ` Cong Wang
0 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2017-09-11 17:35 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, David Miller, Florian Westphal, David Herrmann
On Sun, Sep 10, 2017 at 4:45 AM, Xin Long <lucien.xin@gmail.com> wrote:
> On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long <lucien.xin@gmail.com> wrote:
>>> Now there is no lock protecting nlk ngroups/groups' accessing in
>>> netlink bind and getname. It's safe from nlk groups' setting in
>>> netlink_release, but not from netlink_realloc_groups called by
>>> netlink_setsockopt.
>>>
>>> netlink_lock_table is needed in both netlink bind and getname when
>>> accessing nlk groups.
>>
>> This looks very odd.
>>
>> netlink_lock_table() should be protecting nl_table, why
>> it also protects nlk->groups?? For me it looks like you
>> need lock_sock() instead.
> I believe netlink_lock_table might be only used to protect nl_table
> at the beginning and surely lock_sock is better here. Thanks.
>
> But can you explain why netlink_lock_table() was also used in
> netlink_getsockopt NETLINK_LIST_MEMBERSHIPS ? or it
> was just a mistake ?
No, it is fine but not necessary, because netlink_realloc_groups()
doesn't change nl_table, it only changes nlk->groups. So we
don't have take the global write lock, the lock sock makes more
sense here, same for your bind() and getname() case.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-11 17:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 3:53 [PATCH net] netlink: access nlk groups safely in netlink bind and getname Xin Long
2017-09-07 4:24 ` David Miller
2017-09-08 19:35 ` Cong Wang
2017-09-10 11:45 ` Xin Long
2017-09-11 17:35 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).