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@hartkopp.net
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:
- 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.
- 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
ps. your patches have less context than mine. Do you have different settings for -U<n>, --unified=<n> for 'git diff' ?
if (dev) dev_put(dev); release_sock(sk);
rtnl_unlock(); break;@@ -600,10 +607,16 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
err_mask &= CAN_ERR_MASK;
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_err;}} /* remove current error mask */ if (ro->bound) {@@ -627,6 +640,7 @@ static int raw_setsockopt(struct socket *sock, int level, int optname, dev_put(dev);
release_sock(sk);
rtnl_unlock(); break;