On 07/20/2016 09:17 PM, Willem de Bruijn wrote:
On Wed, Jul 20, 2016 at 3:12 PM, Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
