On 7/21/2021 11:13 PM, Oliver Hartkopp wrote:
>
>
> On 21.07.21 13:37, Ziyang Xuan (William) wrote:
>> On 7/21/2021 5:24 PM, Oliver Hartkopp wrote:
>
>>>
>>> Can you please resend the below patch as suggested by Greg KH and add my
>>>
>>> Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
>>>
>>> as it also adds the dev_get_by_index() return check.
>>>
>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>> index ed4fcb7ab0c3..d3cbc32036c7 100644
>>> --- a/net/can/raw.c
>>> +++ b/net/can/raw.c
>>> @@ -544,14 +544,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>> } else if (count == 1) {
>>> if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
>>> return -EFAULT;
>>> }
>>>
>>> + rtnl_lock();
>>> lock_sock(sk);
>>>
>>> - if (ro->bound && ro->ifindex)
>>> + if (ro->bound && ro->ifindex) {
>>> dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>>> + if (!dev)
>>> + goto out_fil;
>>> + }
>> At first, I also use this modification. After discussion with my partner, we found that
>> it is impossible scenario if we use rtnl_lock to protect net_device object.
>> We can see two sequences:
>> 1. raw_setsockopt first get rtnl_lock, unregister_netdevice_many later.
>> It can be simplified to add the filter in raw_setsockopt, then remove the filter in raw_notify.
>>
>> 2. unregister_netdevice_many first get rtnl_lock, raw_setsockopt later.
>> raw_notify will set ro->ifindex, ro->bound and ro->count to zero firstly. The filter will not
>> be added to any filter_list in raw_notify.
>>
>> So I selected the current modification. Do you think so?
>>
>> My first modification as following:
>>
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index ed4fcb7ab0c3..a0ce4908317f 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -546,10 +546,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>> return -EFAULT;
>> }
>>
>> + rtnl_lock();
>> lock_sock(sk);
>>
>> - if (ro->bound && ro->ifindex)
>> + if (ro->bound && ro->ifindex) {
>> dev = dev_get_by_index(sock_net(sk), ro->ifindex);
>> + if (!dev) {
>> + err = -ENODEV;
>> + goto out_fil;
>> + }
>> + }
>>
>> if (ro->bound) {
>> /* (try to) register the new filters */
>> @@ -559,11 +565,8 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>> else
>> err = raw_enable_filters(sock_net(sk), dev, sk,
>> filter, count);
>> - if (err) {
>> - if (count > 1)
>> - kfree(filter);
>> + if (err)
>> goto out_fil;
>> - }
>>
>> /* remove old filter registrations */
>> raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
>> @@ -584,10 +587,14 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>> ro->count = count;
>>
>> out_fil:
>> + if (err && count > 1)
>> + kfree(filter);
>> +
>
> Setting the err variable to -ENODEV is a good idea but I do not like the movement of kfree(filter).
>
> The kfree() has a tight relation inside the if-statement for ro->bound which makes it easier to understand.
>
> Regards,
> Oliver
I will submit the v2 patch for the problem according to your suggestions. Than you.
