From patchwork Mon Dec 10 22:32:03 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 205050 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id DC7672C03A3 for ; Tue, 11 Dec 2012 09:32:09 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751997Ab2LJWcH (ORCPT ); Mon, 10 Dec 2012 17:32:07 -0500 Received: from mail-da0-f46.google.com ([209.85.210.46]:58768 "EHLO mail-da0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751595Ab2LJWcG (ORCPT ); Mon, 10 Dec 2012 17:32:06 -0500 Received: by mail-da0-f46.google.com with SMTP id p5so1411455dak.19 for ; Mon, 10 Dec 2012 14:32:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; bh=AYIll9GyGAFek4EKzBlK6R1KNXI5NEax32ims8sRBXk=; b=k/eMcKg2uPkl8TcEuA4sQnXybNx7ais3dkzSHZqoGz8t1353k9HNb0FnRVBTfyPXEv hA90NrskNOcu0Ixk9H9kRWmKxBl7ksmZQWVN1PJPqjQWnjgxKxhHB+5sHc599x851b5Q MKGdCwWVvPv/crBUkFacSKL3anHUlxSnP0ILmx93/jRi5be+/JPM3ufPMVWXUaSV36P5 VqsXi+E9GaPqNIR/ozfjABXnH88hQZ8uebFQ+BpuOsp1KjR3w+ZuwfJ2nEgquFLp0K2R 1ZIclDJ+Ot5RVqD9KuFkzzGzz27aE/JhTCa5hqdaJe7WUMvpd54yrn6i2Lq7Hprvm4gc r4NA== Received: by 10.68.248.74 with SMTP id yk10mr43025607pbc.86.1355178725527; Mon, 10 Dec 2012 14:32:05 -0800 (PST) Received: from ?IPv6:2620:0:1000:3304:224:d7ff:fee3:2a94? ([2620:0:1000:3304:224:d7ff:fee3:2a94]) by mx.google.com with ESMTPS id qb3sm805700pbb.35.2012.12.10.14.32.04 (version=SSLv3 cipher=OTHER); Mon, 10 Dec 2012 14:32:04 -0800 (PST) Subject: [PATCH] net: fix a race in gro_cell_poll() From: Eric Dumazet To: Dmitry Kravkov , David Miller Cc: "netdev@vger.kernel.org" In-Reply-To: <1355176551.27891.57.camel@edumazet-glaptop> References: <504C9EFCA2D0054393414C9CB605C37F1BFB80B2@SJEXCHMB06.corp.ad.broadcom.com> <504C9EFCA2D0054393414C9CB605C37F1BFB80D8@SJEXCHMB06.corp.ad.broadcom.com> <1355018645.3113.0.camel@lb-tlvb-dmitry.il.broadcom.com> <1355086161.3113.9.camel@lb-tlvb-dmitry.il.broadcom.com> <504C9EFCA2D0054393414C9CB605C37F1BFC104B@SJEXCHMB06.corp.ad.broadcom.com> <1355158471.27891.44.camel@edumazet-glaptop> <504C9EFCA2D0054393414C9CB605C37F1BFC32A1@SJEXCHMB06.corp.ad.broadcom.com> <1355176551.27891.57.camel@edumazet-glaptop> Date: Mon, 10 Dec 2012 14:32:03 -0800 Message-ID: <1355178723.27891.85.camel@edumazet-glaptop> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Eric Dumazet Dmitry Kravkov reported packet drops for GRE packets since GRO support was added. There is a race in gro_cell_poll() because we call napi_complete() without any synchronization with a concurrent gro_cells_receive() Once bug was triggered, we queued packets but did not schedule NAPI poll. We can fix this issue using the spinlock protected the napi_skbs queue, as we have to hold it to perform skb dequeue anyway. As we open-code skb_dequeue(), we no longer need to mask IRQS, as both producer and consumer run under BH context. Bug added in commit c9e6bc644e (net: add gro_cells infrastructure) Reported-by: Dmitry Kravkov Signed-off-by: Eric Dumazet Tested-by: Dmitry Kravkov --- David: I could reproduce the bug Dmitry reported, and have verified this patch fixes the issue. include/net/gro_cells.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/include/net/gro_cells.h b/include/net/gro_cells.h index 4fd8a4b..e5062c9 100644 --- a/include/net/gro_cells.h +++ b/include/net/gro_cells.h @@ -17,7 +17,6 @@ struct gro_cells { static inline void gro_cells_receive(struct gro_cells *gcells, struct sk_buff *skb) { - unsigned long flags; struct gro_cell *cell = gcells->cells; struct net_device *dev = skb->dev; @@ -35,32 +34,37 @@ static inline void gro_cells_receive(struct gro_cells *gcells, struct sk_buff *s return; } - spin_lock_irqsave(&cell->napi_skbs.lock, flags); + /* We run in BH context */ + spin_lock(&cell->napi_skbs.lock); __skb_queue_tail(&cell->napi_skbs, skb); if (skb_queue_len(&cell->napi_skbs) == 1) napi_schedule(&cell->napi); - spin_unlock_irqrestore(&cell->napi_skbs.lock, flags); + spin_unlock(&cell->napi_skbs.lock); } +/* called unser BH context */ static inline int gro_cell_poll(struct napi_struct *napi, int budget) { struct gro_cell *cell = container_of(napi, struct gro_cell, napi); struct sk_buff *skb; int work_done = 0; + spin_lock(&cell->napi_skbs.lock); while (work_done < budget) { - skb = skb_dequeue(&cell->napi_skbs); + skb = __skb_dequeue(&cell->napi_skbs); if (!skb) break; - + spin_unlock(&cell->napi_skbs.lock); napi_gro_receive(napi, skb); work_done++; + spin_lock(&cell->napi_skbs.lock); } if (work_done < budget) napi_complete(napi); + spin_unlock(&cell->napi_skbs.lock); return work_done; }