[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aa10d178-3421-4759-bac0-2b187255db6f@nokia.com>
Date: Thu, 10 Oct 2024 16:43:34 +0200
From: Stefan Wiehler <stefan.wiehler@...ia.com>
To: Simon Horman <horms@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, David Ahern
<dsahern@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call
in ip6mr_compat_ioctl()
>> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
>> must be done under RCU or RTNL lock. Copy from user space must be
>> performed beforehand as we are not allowed to sleep under RCU lock.
>>
>> Signed-off-by: Stefan Wiehler <stefan.wiehler@...ia.com>
>> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
>> ---
>> v3:
>> - split into separate patches
>> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
>> - rebase on top of net tree
>> - add Fixes tag
>> - refactor out paths
>> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
>> ---
>> net/ipv6/ip6mr.c | 46 ++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 32 insertions(+), 14 deletions(-)
>>
>> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
>> index b18eb4ad21e4..415ba6f55a44 100644
>> --- a/net/ipv6/ip6mr.c
>> +++ b/net/ipv6/ip6mr.c
>> @@ -1961,10 +1961,7 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>> struct mfc6_cache *c;
>> struct net *net = sock_net(sk);
>> struct mr_table *mrt;
>> -
>> - mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
>> - if (!mrt)
>> - return -ENOENT;
>> + int err;
>>
>> switch (cmd) {
>> case SIOCGETMIFCNT_IN6:
>> @@ -1972,8 +1969,30 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>> return -EFAULT;
>> if (vr.mifi >= mrt->maxvif)
>> return -EINVAL;
>
> Hi Stefan,
>
> mrt is now used uninitialised here.
Thanks, that was an accident, it should have stayed where it is.
>> + break;
>> + case SIOCGETSGCNT_IN6:
>> + if (copy_from_user(&sr, arg, sizeof(sr)))
>> + return -EFAULT;
>> + break;
>> + default:
>> + return -ENOIOCTLCMD;
>> + }
>> +
>> +
>> + rcu_read_lock();
>> + mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
>> + if (!mrt) {
>> + err = -ENOENT;
>> + goto out;
>> + }
>> +
>> + switch (cmd) {
>> + case SIOCGETMIFCNT_IN6:
>> + if (vr.mifi >= mrt->maxvif) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>> vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
>> - rcu_read_lock();
>> vif = &mrt->vif_table[vr.mifi];
>> if (VIF_EXISTS(mrt, vr.mifi)) {
>> vr.icount = READ_ONCE(vif->pkt_in);
>
> ...
>
>> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>> return -EFAULT;
>> return 0;
>> }
>> - rcu_read_unlock();
>> - return -EADDRNOTAVAIL;
>> - default:
>> - return -ENOIOCTLCMD;
>> + err = -EADDRNOTAVAIL;
>> + goto out;
>> }
>> +
>
> I think that this out label should be used consistently once rcu_read_lock
> has been taken. With this patch applied there seems to be one case on error
> where rcu_read_unlock() before returning, and one case where it isn't
> (which looks like it leaks the lock).
In the remaining two return paths we need to release the RCU lock before
calling copy_to_user(), so unfortunately we cannot use a common out label.
Kind regards,
Stefan
Powered by blists - more mailing lists