[PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Daniel Borkmann]

During an audit for sk_filter(), we found that rx_busy_skb handling
in l2cap_sock_recv_cb() and l2cap_sock_recvmsg() looks not quite as
intended.

The assumption from commit e328140fdacb ("Bluetooth: Use event-driven
approach for handling ERTM receive buffer") is that errors returned
from sock_queue_rcv_skb() are due to receive buffer shortage. However,
nothing should prevent doing a setsockopt() with SO_ATTACH_FILTER on
the socket, that could drop some of the incoming skbs when handled in
sock_queue_rcv_skb().

In that case sock_queue_rcv_skb() will return with -EPERM, propagated
from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
that we failed due to receive buffer being full. From that point onwards,
due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
due to the filter drop verdict over and over coming from sk_filter().
Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
dropped due to rx_busy_skb being occupied.

Instead, just use __sock_queue_rcv_skb() where an error really tells
that there's a receive buffer issue. Split the sk_filter() and only
enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
skb has already been through the ERTM state machine and it has been
acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
may have consequences wrt future abi, we need to generally disallow
filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
socket initialization. Given that noone run into this issue before as
it otherwise would have been noticed and fixed, there should be little
risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
should there be a use case to call sk_filter() at a safe place for such
kind of sockets.

Fixes: e328140fdacb ("Bluetooth: Use event-driven approach for handling ERTM receive buffer")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Alexei Starovoitov <ast@kernel.org>
---
 I don't have a BT setup at hand, so compile tested only at this time.
 Would be great if some of the BT folks could help out or take over if
 possible. Thanks!

 net/bluetooth/l2cap_sock.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 388ee8b..10ba801 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1019,7 +1019,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
 		goto done;
 
 	if (pi->rx_busy_skb) {
-		if (!sock_queue_rcv_skb(sk, pi->rx_busy_skb))
+		if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
 			pi->rx_busy_skb = NULL;
 		else
 			goto done;
@@ -1270,7 +1270,16 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 		goto done;
 	}
 
-	err = sock_queue_rcv_skb(sk, skb);
+	if (chan->mode != L2CAP_MODE_ERTM) {
+		/* Even if no filter is attached, we could potentially
+		 * get errors from security modules, etc.
+		 */
+		err = sk_filter(sk, skb);
+		if (err)
+			goto done;
+	}
+
+	err = __sock_queue_rcv_skb(sk, skb);
 
 	/* For ERTM, handle one skb that doesn't fit into the recv
 	 * buffer.  This is important to do because the data frames
@@ -1559,6 +1568,9 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
 
 	/* Default config options */
 	chan->flush_to = L2CAP_DEFAULT_FLUSH_TO;
+	/* In ERTM mode, no socket filter can be attached. */
+	if (chan->mode == L2CAP_MODE_ERTM)
+		sock_set_flag(sk, SOCK_FILTER_LOCKED);
 
 	chan->data = sk;
 	chan->ops = &l2cap_chan_ops;
-- 
1.9.3

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

On Wed, 20 Jul 2016, Daniel Borkmann wrote:

> During an audit for sk_filter(), we found that rx_busy_skb handling
> in l2cap_sock_recv_cb() and l2cap_sock_recvmsg() looks not quite as
> intended.
>
> The assumption from commit e328140fdacb ("Bluetooth: Use event-driven
> approach for handling ERTM receive buffer") is that errors returned
> from sock_queue_rcv_skb() are due to receive buffer shortage. However,
> nothing should prevent doing a setsockopt() with SO_ATTACH_FILTER on
> the socket, that could drop some of the incoming skbs when handled in
> sock_queue_rcv_skb().
>
> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
> that we failed due to receive buffer being full. From that point onwards,
> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
> any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
> due to the filter drop verdict over and over coming from sk_filter().
> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
> dropped due to rx_busy_skb being occupied.
>
> Instead, just use __sock_queue_rcv_skb() where an error really tells
> that there's a receive buffer issue. Split the sk_filter() and only
> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
> skb has already been through the ERTM state machine and it has been
> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
> may have consequences wrt future abi, we need to generally disallow
> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
> socket initialization. Given that noone run into this issue before as
> it otherwise would have been noticed and fixed, there should be little
> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
> should there be a use case to call sk_filter() at a safe place for such
> kind of sockets.

I think the location for a call to sk_filter() for ERTM is early in 
l2cap_data_rcv(), if that change is needed later on.

>
> Fixes: e328140fdacb ("Bluetooth: Use event-driven approach for handling ERTM receive buffer")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: Alexei Starovoitov <ast@kernel.org>

Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> ---
> I don't have a BT setup at hand, so compile tested only at this time.
> Would be great if some of the BT folks could help out or take over if
> possible. Thanks!
>
> net/bluetooth/l2cap_sock.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 388ee8b..10ba801 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1019,7 +1019,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> 		goto done;
>
> 	if (pi->rx_busy_skb) {
> -		if (!sock_queue_rcv_skb(sk, pi->rx_busy_skb))
> +		if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))

It would be more future-proof to check specifically for -ENOMEM and 
-ENOBUFS, but right now those are the only errors returned by 
__sock_queue_rcv_skb().

> 			pi->rx_busy_skb = NULL;
> 		else
> 			goto done;
> @@ -1270,7 +1270,16 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
> 		goto done;
> 	}
>
> -	err = sock_queue_rcv_skb(sk, skb);
> +	if (chan->mode != L2CAP_MODE_ERTM) {
> +		/* Even if no filter is attached, we could potentially
> +		 * get errors from security modules, etc.
> +		 */
> +		err = sk_filter(sk, skb);
> +		if (err)
> +			goto done;
> +	}
> +
> +	err = __sock_queue_rcv_skb(sk, skb);
>
> 	/* For ERTM, handle one skb that doesn't fit into the recv
> 	 * buffer.  This is important to do because the data frames

Same minor comment as above (-ENOMEM/-ENOBUFS).

> @@ -1559,6 +1568,9 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent)
>
> 	/* Default config options */
> 	chan->flush_to = L2CAP_DEFAULT_FLUSH_TO;
> +	/* In ERTM mode, no socket filter can be attached. */
> +	if (chan->mode == L2CAP_MODE_ERTM)
> +		sock_set_flag(sk, SOCK_FILTER_LOCKED);
>
> 	chan->data = sk;
> 	chan->ops = &l2cap_chan_ops;

Thanks for the fix!

--
Mat Martineau
Intel OTC

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Daniel Borkmann]

On 07/20/2016 06:47 PM, Mat Martineau wrote:
> On Wed, 20 Jul 2016, Daniel Borkmann wrote:
>
>> During an audit for sk_filter(), we found that rx_busy_skb handling
>> in l2cap_sock_recv_cb() and l2cap_sock_recvmsg() looks not quite as
>> intended.
>>
>> The assumption from commit e328140fdacb ("Bluetooth: Use event-driven
>> approach for handling ERTM receive buffer") is that errors returned
>> from sock_queue_rcv_skb() are due to receive buffer shortage. However,
>> nothing should prevent doing a setsockopt() with SO_ATTACH_FILTER on
>> the socket, that could drop some of the incoming skbs when handled in
>> sock_queue_rcv_skb().
>>
>> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
>> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
>> that we failed due to receive buffer being full. From that point onwards,
>> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
>> any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
>> due to the filter drop verdict over and over coming from sk_filter().
>> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
>> dropped due to rx_busy_skb being occupied.
>>
>> Instead, just use __sock_queue_rcv_skb() where an error really tells
>> that there's a receive buffer issue. Split the sk_filter() and only
>> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
>> skb has already been through the ERTM state machine and it has been
>> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
>> may have consequences wrt future abi, we need to generally disallow
>> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
>> socket initialization. Given that noone run into this issue before as
>> it otherwise would have been noticed and fixed, there should be little
>> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
>> should there be a use case to call sk_filter() at a safe place for such
>> kind of sockets.
>
> I think the location for a call to sk_filter() for ERTM is early in l2cap_data_rcv(), if that change is needed later on.
>
>> Fixes: e328140fdacb ("Bluetooth: Use event-driven approach for handling ERTM receive buffer")
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> Cc: Willem de Bruijn <willemb@google.com>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
>> ---
>> I don't have a BT setup at hand, so compile tested only at this time.
>> Would be great if some of the BT folks could help out or take over if
>> possible. Thanks!
>>
>> net/bluetooth/l2cap_sock.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> index 388ee8b..10ba801 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -1019,7 +1019,7 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>>         goto done;
>>
>>     if (pi->rx_busy_skb) {
>> -        if (!sock_queue_rcv_skb(sk, pi->rx_busy_skb))
>> +        if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
>
> It would be more future-proof to check specifically for -ENOMEM and -ENOBUFS, but right now those are the only errors returned by __sock_queue_rcv_skb().

Since there's also core code relying that an error from __sock_queue_rcv_skb()
really means we have some recvbuf issue, I think we can spare making this two
tests in fast-path.

Thanks,
Daniel

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Willem de Bruijn]

> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
> that we failed due to receive buffer being full. From that point onwards,
> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
> any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
> due to the filter drop verdict over and over coming from sk_filter().
> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
> dropped due to rx_busy_skb being occupied.
>
> Instead, just use __sock_queue_rcv_skb() where an error really tells
> that there's a receive buffer issue. Split the sk_filter() and only
> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
> skb has already been through the ERTM state machine and it has been
> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
> may have consequences wrt future abi, we need to generally disallow
> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
> socket initialization. Given that noone run into this issue before as
> it otherwise would have been noticed and fixed, there should be little
> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
> should there be a use case to call sk_filter() at a safe place for such
> kind of sockets.

Silently setting SOCK_FILTER_LOCKED is unexpected and inconsistent
with all other socket types. I don't think that we need to protect
processes in this way against their own actions.

All socket types support SO_ATTACH_FILTER and there is value in being
able to expect consistent behavior across sockets for SOL_SOCKET
options. Even if using the feature incorrectly can cause a protocol to
become wedged.

Even without this precaution, this patch fixes the real wedge: an
infinite rx_busy_skb filter loop. It is a stretch, but I could imagine
scenarios where a protocol wants to acknowledge data arrival, but drop
contents instead of queue it to userspace.

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Daniel Borkmann]

On 07/20/2016 08:57 PM, Willem de Bruijn wrote:
>> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
>> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
>> that we failed due to receive buffer being full. From that point onwards,
>> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
>> any forward progress as rx_busy_skb is never cleared from l2cap_sock_recvmsg(),
>> due to the filter drop verdict over and over coming from sk_filter().
>> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
>> dropped due to rx_busy_skb being occupied.
>>
>> Instead, just use __sock_queue_rcv_skb() where an error really tells
>> that there's a receive buffer issue. Split the sk_filter() and only
>> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
>> skb has already been through the ERTM state machine and it has been
>> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
>> may have consequences wrt future abi, we need to generally disallow
>> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
>> socket initialization. Given that noone run into this issue before as
>> it otherwise would have been noticed and fixed, there should be little
>> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
>> should there be a use case to call sk_filter() at a safe place for such
>> kind of sockets.
>
> Silently setting SOCK_FILTER_LOCKED is unexpected and inconsistent
> with all other socket types. I don't think that we need to protect
> processes in this way against their own actions.
>
> All socket types support SO_ATTACH_FILTER and there is value in being
> able to expect consistent behavior across sockets for SOL_SOCKET
> options. Even if using the feature incorrectly can cause a protocol to
> become wedged.
>
> Even without this precaution, this patch fixes the real wedge: an
> infinite rx_busy_skb filter loop. It is a stretch, but I could imagine
> scenarios where a protocol wants to acknowledge data arrival, but drop
> contents instead of queue it to userspace.

Right, I was thinking that when sk_filter() is being ignored silently for
ERTM modes (which would be the case w/o setting the option), then if an
sk_filter() later on should be placed to some location that seems a better
spot, we might change user behavior. Right now it seems noone has tried
it out, otherwise as said it would have been noticed. If we lock it, it
can still be adapted later on. Otoh, if someone has a BT test setup and is
more familiar with ERTM, then there should be no issue moving this to a
more appropriate location in the first place perhaps.

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Willem de Bruijn]

On Wed, Jul 20, 2016 at 3:12 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 07/20/2016 08:57 PM, Willem de Bruijn wrote:
>>>
>>> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
>>> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
>>> that we failed due to receive buffer being full. From that point onwards,
>>> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
>>> any forward progress as rx_busy_skb is never cleared from
>>> l2cap_sock_recvmsg(),
>>> due to the filter drop verdict over and over coming from sk_filter().
>>> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
>>> dropped due to rx_busy_skb being occupied.
>>>
>>> Instead, just use __sock_queue_rcv_skb() where an error really tells
>>> that there's a receive buffer issue. Split the sk_filter() and only
>>> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
>>> skb has already been through the ERTM state machine and it has been
>>> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
>>> may have consequences wrt future abi, we need to generally disallow
>>> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
>>> socket initialization. Given that noone run into this issue before as
>>> it otherwise would have been noticed and fixed, there should be little
>>> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
>>> should there be a use case to call sk_filter() at a safe place for such
>>> kind of sockets.
>>
>>
>> Silently setting SOCK_FILTER_LOCKED is unexpected and inconsistent
>> with all other socket types. I don't think that we need to protect
>> processes in this way against their own actions.
>>
>> All socket types support SO_ATTACH_FILTER and there is value in being
>> able to expect consistent behavior across sockets for SOL_SOCKET
>> options. Even if using the feature incorrectly can cause a protocol to
>> become wedged.
>>
>> Even without this precaution, this patch fixes the real wedge: an
>> infinite rx_busy_skb filter loop. It is a stretch, but I could imagine
>> scenarios where a protocol wants to acknowledge data arrival, but drop
>> contents instead of queue it to userspace.
>
>
> Right, I was thinking that when sk_filter() is being ignored silently for
> ERTM modes (which would be the case w/o setting the option),

But only because of the explicit branch on chan_mode, right? When
eschewing that (as in your earlier suggestion), filtering works as
expected while it will no longer block the protocol as the packet is
not requeued.

> then if an
> sk_filter() later on should be placed to some location that seems a better
> spot, we might change user behavior. Right now it seems noone has tried
> it out, otherwise as said it would have been noticed. If we lock it, it
> can still be adapted later on. Otoh, if someone has a BT test setup and is
> more familiar with ERTM, then there should be no issue moving this to a
> more appropriate location in the first place perhaps.

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Daniel Borkmann]

On 07/20/2016 09:17 PM, Willem de Bruijn wrote:
> On Wed, Jul 20, 2016 at 3:12 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 07/20/2016 08:57 PM, Willem de Bruijn wrote:
>>>>
>>>> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
>>>> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
>>>> that we failed due to receive buffer being full. From that point onwards,
>>>> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
>>>> any forward progress as rx_busy_skb is never cleared from
>>>> l2cap_sock_recvmsg(),
>>>> due to the filter drop verdict over and over coming from sk_filter().
>>>> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
>>>> dropped due to rx_busy_skb being occupied.
>>>>
>>>> Instead, just use __sock_queue_rcv_skb() where an error really tells
>>>> that there's a receive buffer issue. Split the sk_filter() and only
>>>> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
>>>> skb has already been through the ERTM state machine and it has been
>>>> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
>>>> may have consequences wrt future abi, we need to generally disallow
>>>> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
>>>> socket initialization. Given that noone run into this issue before as
>>>> it otherwise would have been noticed and fixed, there should be little
>>>> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
>>>> should there be a use case to call sk_filter() at a safe place for such
>>>> kind of sockets.
>>>
>>> Silently setting SOCK_FILTER_LOCKED is unexpected and inconsistent
>>> with all other socket types. I don't think that we need to protect
>>> processes in this way against their own actions.
>>>
>>> All socket types support SO_ATTACH_FILTER and there is value in being
>>> able to expect consistent behavior across sockets for SOL_SOCKET
>>> options. Even if using the feature incorrectly can cause a protocol to
>>> become wedged.
>>>
>>> Even without this precaution, this patch fixes the real wedge: an
>>> infinite rx_busy_skb filter loop. It is a stretch, but I could imagine
>>> scenarios where a protocol wants to acknowledge data arrival, but drop
>>> contents instead of queue it to userspace.
>>
>> Right, I was thinking that when sk_filter() is being ignored silently for
>> ERTM modes (which would be the case w/o setting the option),
>
> But only because of the explicit branch on chan_mode, right? When

Correct.

> eschewing that (as in your earlier suggestion), filtering works as
> expected while it will no longer block the protocol as the packet is
> not requeued.

I was told that skbs at this point in the path cannot be discarded w/o
shutting down the whole connection as they already went through the
state machine. Mat, thoughts?

>> then if an
>> sk_filter() later on should be placed to some location that seems a better
>> spot, we might change user behavior. Right now it seems noone has tried
>> it out, otherwise as said it would have been noticed. If we lock it, it
>> can still be adapted later on. Otoh, if someone has a BT test setup and is
>> more familiar with ERTM, then there should be no issue moving this to a
>> more appropriate location in the first place perhaps.

Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb

[Mat Martineau]

Daniel -

On Wed, 20 Jul 2016, Daniel Borkmann wrote:

> On 07/20/2016 09:17 PM, Willem de Bruijn wrote:
>> On Wed, Jul 20, 2016 at 3:12 PM, Daniel Borkmann <daniel@iogearbox.net> 
>> wrote:
>>> On 07/20/2016 08:57 PM, Willem de Bruijn wrote:
>>>>> 
>>>>> In that case sock_queue_rcv_skb() will return with -EPERM, propagated
>>>>> from sk_filter() and if in L2CAP_MODE_ERTM mode, wrong assumption was
>>>>> that we failed due to receive buffer being full. From that point 
>>>>> onwards,
>>>>> due to the to-be-dropped skb being held in rx_busy_skb, we cannot make
>>>>> any forward progress as rx_busy_skb is never cleared from
>>>>> l2cap_sock_recvmsg(),
>>>>> due to the filter drop verdict over and over coming from sk_filter().
>>>>> Meanwhile, in l2cap_sock_recv_cb() all new incoming skbs are being
>>>>> dropped due to rx_busy_skb being occupied.
>>>>> 
>>>>> Instead, just use __sock_queue_rcv_skb() where an error really tells
>>>>> that there's a receive buffer issue. Split the sk_filter() and only
>>>>> enable it for non-L2CAP_MODE_ERTM modes since at this point in time the
>>>>> skb has already been through the ERTM state machine and it has been
>>>>> acked, so dropping is not allowed. Since skipping sk_filter() for ERTM
>>>>> may have consequences wrt future abi, we need to generally disallow
>>>>> filters to be attached for this mode. So set SOCK_FILTER_LOCKED during
>>>>> socket initialization. Given that noone run into this issue before as
>>>>> it otherwise would have been noticed and fixed, there should be little
>>>>> risk of any breakage. The SOCK_FILTER_LOCKED can later on be lifted
>>>>> should there be a use case to call sk_filter() at a safe place for such
>>>>> kind of sockets.
>>>> 
>>>> Silently setting SOCK_FILTER_LOCKED is unexpected and inconsistent
>>>> with all other socket types. I don't think that we need to protect
>>>> processes in this way against their own actions.
>>>> 
>>>> All socket types support SO_ATTACH_FILTER and there is value in being
>>>> able to expect consistent behavior across sockets for SOL_SOCKET
>>>> options. Even if using the feature incorrectly can cause a protocol to
>>>> become wedged.
>>>> 
>>>> Even without this precaution, this patch fixes the real wedge: an
>>>> infinite rx_busy_skb filter loop. It is a stretch, but I could imagine
>>>> scenarios where a protocol wants to acknowledge data arrival, but drop
>>>> contents instead of queue it to userspace.
>>> 
>>> Right, I was thinking that when sk_filter() is being ignored silently for
>>> ERTM modes (which would be the case w/o setting the option),
>> 
>> But only because of the explicit branch on chan_mode, right? When
>
> Correct.
>
>> eschewing that (as in your earlier suggestion), filtering works as
>> expected while it will no longer block the protocol as the packet is
>> not requeued.
>
> I was told that skbs at this point in the path cannot be discarded w/o
> shutting down the whole connection as they already went through the
> state machine. Mat, thoughts?

ERTM is a reliable protocol like TCP, and upper layer protocols or 
applications can't handle random chunks of data disappearing from the 
stream that is read from the socket.

I saw that TCP calls sk_filter() before received packets are passed to 
tcp_v{4,6}_do_rcv(). It looks like filtered TCP packets will not be acked 
and the stream read by userspace is never missing data. ERTM needs to work 
similarly.

>>> then if an
>>> sk_filter() later on should be placed to some location that seems a better
>>> spot, we might change user behavior. Right now it seems noone has tried
>>> it out, otherwise as said it would have been noticed. If we lock it, it
>>> can still be adapted later on. Otoh, if someone has a BT test setup and is
>>> more familiar with ERTM, then there should be no issue moving this to a
>>> more appropriate location in the first place perhaps.
>

--
Mat Martineau
Intel OTC