2012-06-06 14:43:28

by Andrei Emeltchenko

Subject: [RFC] Bluetooth: Lock l2cap chan in sockopt

From: Andrei Emeltchenko <[email protected]>


Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_sock.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a4bb27e..373ce9c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -275,6 +275,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
if (get_user(len, optlen))
return -EFAULT;

+ l2cap_chan_lock(chan);
lock_sock(sk);

switch (optname) {
@@ -345,6 +346,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
}

release_sock(sk);
+ l2cap_chan_unlock(chan);
+
return err;
}

@@ -367,6 +370,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
if (get_user(len, optlen))
return -EFAULT;

+ l2cap_chan_lock(chan);
lock_sock(sk);

switch (optname) {
@@ -442,6 +446,8 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
}

release_sock(sk);
+ l2cap_chan_unlock(chan);
+
return err;
}

@@ -471,6 +477,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us

BT_DBG("sk %p", sk);

+ l2cap_chan_lock(chan);
lock_sock(sk);

switch (optname) {
@@ -556,6 +563,8 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
}

release_sock(sk);
+ l2cap_chan_unlock(chan);
+
return err;
}

@@ -577,6 +586,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
if (level != SOL_BLUETOOTH)
return -ENOPROTOOPT;

+ l2cap_chan_lock(chan);
lock_sock(sk);

switch (optname) {
@@ -729,6 +739,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
}

release_sock(sk);
+ l2cap_chan_unlock(chan);
+
return err;
}

--
1.7.9.5

2012-06-11 08:20:51

by Andrei Emeltchenko

Subject: Re: [RFC] Bluetooth: Lock l2cap chan in sockopt

Hi Mat,

On Fri, Jun 08, 2012 at 11:02:47AM -0700, Mat Martineau wrote:
> >Signed-off-by: Andrei Emeltchenko <[email protected]>
>
> Commit message is quite short :) (I know, it's just an RFC)

Maybe I need to put one oops dump, but I was not able to reproduce it.

[ 745.772484] BUG: unable to handle kernel NULL pointer dereference at
(null)
[ 745.774157] IP: [<f828dccc>] l2cap_sock_getsockopt+0x2ec/0x3f0
[bluetooth]
[ 745.775077] *pde = 00000000
[ 745.776483] Oops: 0000 [#1] SMP


Best regards
Andrei Emeltchenko

2012-06-08 18:02:47

by Mat Martineau

Subject: Re: [RFC] Bluetooth: Lock l2cap chan in sockopt

Andrei -

On Wed, 6 Jun 2012, Andrei Emeltchenko wrote:

> From: Andrei Emeltchenko <[email protected]>
>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>

Commit message is quite short :) (I know, it's just an RFC)

> ---
> net/bluetooth/l2cap_sock.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index a4bb27e..373ce9c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -275,6 +275,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
> if (get_user(len, optlen))
> return -EFAULT;
>
> + l2cap_chan_lock(chan);
> lock_sock(sk);
>
> switch (optname) {
> @@ -345,6 +346,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
> }
>
> release_sock(sk);
> + l2cap_chan_unlock(chan);
> +
> return err;
> }
>
> @@ -367,6 +370,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
> if (get_user(len, optlen))
> return -EFAULT;
>
> + l2cap_chan_lock(chan);
> lock_sock(sk);
>
> switch (optname) {
> @@ -442,6 +446,8 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
> }
>
> release_sock(sk);
> + l2cap_chan_unlock(chan);
> +
> return err;
> }
>
> @@ -471,6 +477,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
>
> BT_DBG("sk %p", sk);
>
> + l2cap_chan_lock(chan);
> lock_sock(sk);
>
> switch (optname) {
> @@ -556,6 +563,8 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
> }
>
> release_sock(sk);
> + l2cap_chan_unlock(chan);
> +
> return err;
> }
>
> @@ -577,6 +586,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
> if (level != SOL_BLUETOOTH)
> return -ENOPROTOOPT;
>
> + l2cap_chan_lock(chan);
> lock_sock(sk);
>
> switch (optname) {
> @@ -729,6 +739,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
> }
>
> release_sock(sk);
> + l2cap_chan_unlock(chan);
> +
> return err;
> }
>
> --
> 1.7.9.5

Code looks good to me. In the near future, BT_CHANNEL_POLICY will
call in to the L2CAP core to do channel moves, but I think it helps to
already have the lock held in that case.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum