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

On Sun, Sep 10, 2017 at 4:45 AM, Xin Long [off-list ref] wrote:

On Sat, Sep 9, 2017 at 7:35 AM, Cong Wang [off-list ref] wrote:

quoted

On Tue, Sep 5, 2017 at 8:53 PM, Xin Long [off-list ref] wrote:

quoted

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.