Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

2017-09-11 Thread Cong Wang 
On Sun, Sep 10, 2017 at 4:45 AM, Xin Long  wrote:
> On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang  wrote:
>> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long  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.

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

2017-09-10 Thread Xin Long 
On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang  wrote:
> On Tue, Sep 5, 2017 at 8:53 PM, Xin Long  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 ?

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

2017-09-08 Thread Cong Wang 
On Tue, Sep 5, 2017 at 8:53 PM, Xin Long  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.

Re: [PATCH net] netlink: access nlk groups safely in netlink bind and getname

2017-09-06 Thread David Miller 
From: Xin Long 
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 
> Signed-off-by: Xin Long 

Applied.