Re: [PATCH net-next v2 8/9] net: ipmr: rearrange and cleanup setsockopt

[Nikolay Aleksandrov]

On 11/23/2015 06:44 AM, Cong Wang wrote:
> On Sat, Nov 21, 2015 at 6:57 AM, Nikolay Aleksandrov
>  wrote:
>>  net/ipv4/ipmr.c | 191 
>> +++-
>>  1 file changed, 107 insertions(+), 84 deletions(-)
> 
> Does this really simplify the code? :-/
> 
Did I really say it does ? :-) Now, to the point it just makes it
much easier to reason about this setsockopt which was doing conditional
locking in some of the cases before, and some were left out, also "v"
was sometimes signed and sometimes unsigned, it's clearer now which type
is used. I've left a comment why the only special case needs to unlock
rtnl (MRT_DONE).



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next v2 8/9] net: ipmr: rearrange and cleanup setsockopt

[Cong Wang]

On Sat, Nov 21, 2015 at 6:57 AM, Nikolay Aleksandrov
 wrote:
>  net/ipv4/ipmr.c | 191 
> +++-
>  1 file changed, 107 insertions(+), 84 deletions(-)

Does this really simplify the code? :-/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next v2 8/9] net: ipmr: rearrange and cleanup setsockopt

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov 

Take rtnl in the beginning unconditionally as most options already need
it (one exception - MRT_DONE, see the comment inside), make the
lock/unlock places central and move out the switch() local variables.

Signed-off-by: Nikolay Aleksandrov 
---
 net/ipv4/ipmr.c | 191 +++-
 1 file changed, 107 insertions(+), 84 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 50aec313119d..e384f39202cb 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1276,38 +1276,45 @@ static void mrtsock_destruct(struct sock *sk)
  * MOSPF/PIM router set up we can clean this up.
  */
 
-int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, 
unsigned int optlen)
+int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval,
+unsigned int optlen)
 {
-   int ret, parent = 0;
-   struct vifctl vif;
-   struct mfcctl mfc;
struct net *net = sock_net(sk);
+   int val, ret = 0, parent = 0;
struct mr_table *mrt;
+   struct vifctl vif;
+   struct mfcctl mfc;
+   u32 uval;
 
+   /* There's one exception to the lock - MRT_DONE which needs to unlock */
+   rtnl_lock();
if (sk->sk_type != SOCK_RAW ||
-   inet_sk(sk)->inet_num != IPPROTO_IGMP)
-   return -EOPNOTSUPP;
+   inet_sk(sk)->inet_num != IPPROTO_IGMP) {
+   ret = -EOPNOTSUPP;
+   goto out_unlock;
+   }
 
mrt = ipmr_get_table(net, raw_sk(sk)->ipmr_table ? : RT_TABLE_DEFAULT);
-   if (!mrt)
-   return -ENOENT;
-
+   if (!mrt) {
+   ret = -ENOENT;
+   goto out_unlock;
+   }
if (optname != MRT_INIT) {
if (sk != rcu_access_pointer(mrt->mroute_sk) &&
-   !ns_capable(net->user_ns, CAP_NET_ADMIN))
-   return -EACCES;
+   !ns_capable(net->user_ns, CAP_NET_ADMIN)) {
+   ret = -EACCES;
+   goto out_unlock;
+   }
}
 
switch (optname) {
case MRT_INIT:
if (optlen != sizeof(int))
-   return -EINVAL;
-
-   rtnl_lock();
-   if (rtnl_dereference(mrt->mroute_sk)) {
-   rtnl_unlock();
-   return -EADDRINUSE;
-   }
+   ret = -EINVAL;
+   if (rtnl_dereference(mrt->mroute_sk))
+   ret = -EADDRINUSE;
+   if (ret)
+   break;
 
ret = ip_ra_control(sk, 1, mrtsock_destruct);
if (ret == 0) {
@@ -1317,30 +1324,41 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, 
char __user *optval, unsi
NETCONFA_IFINDEX_ALL,
net->ipv4.devconf_all);
}
-   rtnl_unlock();
-   return ret;
+   break;
case MRT_DONE:
-   if (sk != rcu_access_pointer(mrt->mroute_sk))
-   return -EACCES;
-   return ip_ra_control(sk, 0, NULL);
+   if (sk != rcu_access_pointer(mrt->mroute_sk)) {
+   ret = -EACCES;
+   } else {
+   /* We need to unlock here because mrtsock_destruct takes
+* care of rtnl itself and we can't change that due to
+* the IP_ROUTER_ALERT setsockopt which runs without it.
+*/
+   rtnl_unlock();
+   ret = ip_ra_control(sk, 0, NULL);
+   goto out;
+   }
+   break;
case MRT_ADD_VIF:
case MRT_DEL_VIF:
-   if (optlen != sizeof(vif))
-   return -EINVAL;
-   if (copy_from_user(, optval, sizeof(vif)))
-   return -EFAULT;
-   if (vif.vifc_vifi >= MAXVIFS)
-   return -ENFILE;
-   rtnl_lock();
+   if (optlen != sizeof(vif)) {
+   ret = -EINVAL;
+   break;
+   }
+   if (copy_from_user(, optval, sizeof(vif))) {
+   ret = -EFAULT;
+   break;
+   }
+   if (vif.vifc_vifi >= MAXVIFS) {
+   ret = -ENFILE;
+   break;
+   }
if (optname == MRT_ADD_VIF) {
ret = vif_add(net, mrt, ,
  sk == rtnl_dereference(mrt->mroute_sk));
} else {
ret = vif_delete(mrt, vif.vifc_vifi, 0, NULL);
}
-   rtnl_unlock();
-   return