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 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. -- 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
