| Message ID | 1273409123.2727.387.camel@ibex |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Andrew Hendry <andrew.hendry@gmail.com> Date: Sun, 09 May 2010 22:45:23 +1000 > @@ -465,20 +464,20 @@ static int x25_setsockopt(struct socket *sock, int level, int optname, > if (get_user(opt, (int __user *)optval)) > goto out; > > + lock_sock(sk); > x25_sk(sk)->qbitincl = !!opt; > + release_sock(sk); This is completely bogus. A store is always atomic on an SMP system, and "opt" is in a local variable rather than being computed based upon some memory values. There is no reason to require locking for this operation. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le dimanche 09 mai 2010 à 18:46 -0700, David Miller a écrit : > From: Andrew Hendry <andrew.hendry@gmail.com> > Date: Sun, 09 May 2010 22:45:23 +1000 > > > @@ -465,20 +464,20 @@ static int x25_setsockopt(struct socket *sock, int level, int optname, > > if (get_user(opt, (int __user *)optval)) > > goto out; > > > > + lock_sock(sk); > > x25_sk(sk)->qbitincl = !!opt; > > + release_sock(sk); > > This is completely bogus. > > A store is always atomic on an SMP system, and "opt" is in a local variable > rather than being computed based upon some memory values. > > There is no reason to require locking for this operation. Well, its probably better than lock_kernel() ;) qbitincl is a char, I suspect some arches cant store a char in an atomic way ? Alpha comes to mind. We now have lock_sock_bh()/unlock_sock_bh() for this kind of very short sections, where we cant sleep. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 10 May 2010 06:50:40 +0200 > Well, its probably better than lock_kernel() ;) > > qbitincl is a char, I suspect some arches cant store a char in an atomic > way ? Alpha comes to mind. True. > We now have lock_sock_bh()/unlock_sock_bh() for this kind of very short > sections, where we cant sleep. Another option is to simply use an atomic bitmask to represent these booleans instead of a crufty set of chars. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c index 296e65e..9f177a1 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -453,7 +453,6 @@ static int x25_setsockopt(struct socket *sock, int level, int optname, struct sock *sk = sock->sk; int rc = -ENOPROTOOPT; - lock_kernel(); if (level != SOL_X25 || optname != X25_QBITINCL) goto out; @@ -465,20 +464,20 @@ static int x25_setsockopt(struct socket *sock, int level, int optname, if (get_user(opt, (int __user *)optval)) goto out; + lock_sock(sk); x25_sk(sk)->qbitincl = !!opt; + release_sock(sk); rc = 0; out: - unlock_kernel(); return rc; } static int x25_getsockopt(struct socket *sock, int level, int optname, char __user *optval, int __user *optlen) { - struct sock *sk = sock->sk; + struct x25_sock *sk = x25_sk(sock->sk); int val, len, rc = -ENOPROTOOPT; - lock_kernel(); if (level != SOL_X25 || optname != X25_QBITINCL) goto out; @@ -496,10 +495,9 @@ static int x25_getsockopt(struct socket *sock, int level, int optname, if (put_user(len, optlen)) goto out; - val = x25_sk(sk)->qbitincl; + val = sk->qbitincl; rc = copy_to_user(optval, &val, len) ? -EFAULT : 0; out: - unlock_kernel(); return rc; }
x25_setsockopt only updates the socket x25_get only reads Signed-off-by: Andrew Hendry <andrew.hendry@gmail.com> --- net/x25/af_x25.c | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-)