Return-Path: Date: Wed, 20 Jul 2016 14:02:48 -0700 (PDT) From: Mat Martineau To: Daniel Borkmann cc: Willem de Bruijn , johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, mathew.j.martineau@linux.intel.com, Marcel Holtmann , Gustavo Padovan , Alexei Starovoitov Subject: Re: [PATCH net RFT] bluetooth, bpf: split sk_filter in l2cap_sock_recv_cb In-Reply-To: <578FDA02.6070401@iogearbox.net> Message-ID: References: <578FCD20.1040109@iogearbox.net> <578FDA02.6070401@iogearbox.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed List-ID: 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 >> 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