| Message ID | 20231201143821.1091005-1-aleksander.lobakin@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl] idpf: fix corrupted frames and skb leaks in singleq mode | expand |
On Fri, Dec 01, 2023 at 03:38:21PM +0100, Alexander Lobakin wrote: > idpf_ring::skb serves only for keeping an incomplete frame between > several NAPI Rx polling cycles, as one cycle may end up before > processing the end of packet descriptor. The pointer is taken from > the ring onto the stack before entering the loop and gets written > there after the loop exits. When inside the loop, only the onstack > pointer is used. > For some reason, the logics is broken in the singleq mode, where the > pointer is taken from the ring each iteration. This means that if a > frame got fragmented into several descriptors, each fragment will have > its own skb, but only the last one will be passed up the stack > (containing garbage), leaving the rest leaked. > Just don't touch the ring skb field inside the polling loop, letting > the onstack skb pointer work as expected: build a new skb if it's the > first frame descriptor and attach a frag otherwise. > > Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> Reviewed-by: Simon Horman <horms@kernel.org>
On Thu, Dec 7, 2023 at 12:58 PM Simon Horman <horms@kernel.org> wrote: > > On Fri, Dec 01, 2023 at 03:38:21PM +0100, Alexander Lobakin wrote: > > idpf_ring::skb serves only for keeping an incomplete frame between > > several NAPI Rx polling cycles, as one cycle may end up before > > processing the end of packet descriptor. The pointer is taken from > > the ring onto the stack before entering the loop and gets written > > there after the loop exits. When inside the loop, only the onstack > > pointer is used. > > For some reason, the logics is broken in the singleq mode, where the > > pointer is taken from the ring each iteration. This means that if a > > frame got fragmented into several descriptors, each fragment will have > > its own skb, but only the last one will be passed up the stack > > (containing garbage), leaving the rest leaked. > > Just don't touch the ring skb field inside the polling loop, letting > > the onstack skb pointer work as expected: build a new skb if it's the > > first frame descriptor and attach a frag otherwise. > > > > Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll") > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > > Reviewed-by: Simon Horman <horms@kernel.org> It seems singlequeue mode is not really used on idpf :) Reviewed-by: Eric Dumazet <edumazet@google.com>
From: Eric Dumazet <edumazet@google.com> Date: Thu, 7 Dec 2023 13:48:52 +0100 > On Thu, Dec 7, 2023 at 12:58 PM Simon Horman <horms@kernel.org> wrote: >> >> On Fri, Dec 01, 2023 at 03:38:21PM +0100, Alexander Lobakin wrote: >>> idpf_ring::skb serves only for keeping an incomplete frame between >>> several NAPI Rx polling cycles, as one cycle may end up before >>> processing the end of packet descriptor. The pointer is taken from >>> the ring onto the stack before entering the loop and gets written >>> there after the loop exits. When inside the loop, only the onstack >>> pointer is used. >>> For some reason, the logics is broken in the singleq mode, where the >>> pointer is taken from the ring each iteration. This means that if a >>> frame got fragmented into several descriptors, each fragment will have >>> its own skb, but only the last one will be passed up the stack >>> (containing garbage), leaving the rest leaked. >>> Just don't touch the ring skb field inside the polling loop, letting >>> the onstack skb pointer work as expected: build a new skb if it's the >>> first frame descriptor and attach a frag otherwise. >>> >>> Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll") >>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> >>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> >> >> Reviewed-by: Simon Horman <horms@kernel.org> > > It seems singlequeue mode is not really used on idpf :) From what I know, there's currently no hardware supporting singleq mode. I'd remove it completely, but seems like it was decided to keep it in case someone would like to support it one day... > > Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks, Olek
diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c index 81288a17da2a..20c4b3a64710 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c @@ -1044,7 +1044,6 @@ static int idpf_rx_singleq_clean(struct idpf_queue *rx_q, int budget) } idpf_rx_sync_for_cpu(rx_buf, fields.size); - skb = rx_q->skb; if (skb) idpf_rx_add_frag(rx_buf, skb, fields.size); else
idpf_ring::skb serves only for keeping an incomplete frame between several NAPI Rx polling cycles, as one cycle may end up before processing the end of packet descriptor. The pointer is taken from the ring onto the stack before entering the loop and gets written there after the loop exits. When inside the loop, only the onstack pointer is used. For some reason, the logics is broken in the singleq mode, where the pointer is taken from the ring each iteration. This means that if a frame got fragmented into several descriptors, each fragment will have its own skb, but only the last one will be passed up the stack (containing garbage), leaving the rest leaked. Just don't touch the ring skb field inside the polling loop, letting the onstack skb pointer work as expected: build a new skb if it's the first frame descriptor and attach a frag otherwise. Fixes: a5ab9ee0df0b ("idpf: add singleq start_xmit and napi poll") Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c | 1 - 1 file changed, 1 deletion(-)