Received-SPF: pass (google.com: domain of linux-kernel+bounces-125549-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Dexuan Cui <decui@microsoft.com>
To: Haiyang Zhang <haiyangz@microsoft.com>, "linux-hyperv@vger.kernel.org"
	<linux-hyperv@vger.kernel.org>, "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>
CC: stephen <stephen@networkplumber.org>, KY Srinivasan <kys@microsoft.com>,
	Paul Rosswurm <paulros@microsoft.com>, "olaf@aepfle.de" <olaf@aepfle.de>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>, "davem@davemloft.net"
	<davem@davemloft.net>, "wei.liu@kernel.org" <wei.liu@kernel.org>,
	"edumazet@google.com" <edumazet@google.com>, "kuba@kernel.org"
	<kuba@kernel.org>, "pabeni@redhat.com" <pabeni@redhat.com>, "leon@kernel.org"
	<leon@kernel.org>, Long Li <longli@microsoft.com>,
	"ssengar@linux.microsoft.com" <ssengar@linux.microsoft.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>, "john.fastabend@gmail.com"
	<john.fastabend@gmail.com>, "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"ast@kernel.org" <ast@kernel.org>, "sharmaajay@microsoft.com"
	<sharmaajay@microsoft.com>, "hawk@kernel.org" <hawk@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>, "shradhagupta@linux.microsoft.com"
	<shradhagupta@linux.microsoft.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "stable@vger.kernel.org"
	<stable@vger.kernel.org>
Subject: RE: [PATCH net] net: mana: Fix Rx DMA datasize and skb_over_panic
Thread-Topic: [PATCH net] net: mana: Fix Rx DMA datasize and skb_over_panic
Thread-Index: AQHagiFTbDDISYHohEGBGDLC4qicc7FPUWYQ
Date: Sat, 30 Mar 2024 00:12:00 +0000
Message-ID:
 <CY5PR21MB375904FD3437BA610E6BDBD1BF392@CY5PR21MB3759.namprd21.prod.outlook.com>
References: <1711748213-30517-1-git-send-email-haiyangz@microsoft.com>
In-Reply-To: <1711748213-30517-1-git-send-email-haiyangz@microsoft.com>
Accept-Language: en-US
Content-Language: en-US
msip_labels:
 MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ActionId=59ee9ed2-1d2f-476e-81a2-fdbc522a1f8e;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=true;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=Internal;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2024-03-29T22:45:42Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;
x-ms-publictraffictype: Email
x-ms-traffictypediagnostic: CY5PR21MB3759:EE_|CH2PR21MB1509:EE_
x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr
x-ms-exchange-senderadcheck: 1
x-ms-exchange-antispam-relay: 0
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info:
 78t04tPeAiIH8qy0QFz5N7v2Xsnhe3pj5jFkJbPHhwCJOHsBaSpWJ4TT9I24HakOElOqoLTMH9BSOcd7/7z3iNk6BGSSu4ZZpDaY1rKd+eJqQ7SKjCb/WyY4qkdyRmWBD7/PpiGtnsNHE2agjJycjJMhwYsmcv8ZwdETvaT7+/VffRhWkui94LoMj7QyfXxKhwr2+hPGDg2FkTdunZ4BCoQ3lhZvI2Z556p2OwyoaIEEfmJNE3rVk1Z89S7av6Hz5ocIgNDntW+kACrWJyTTnidrtqSudjuDErFzRIQZjGSQyEh5SLi7eehwMmXWiyJTsoQbErnYR0pS97+fkn+WpGdAFkzdKwrHfIAQKFxDaX45LLOlxYJRTt13YhRodULcsxI9enZM82HYCaUotRTDioUVn+9NFRM9AtDazB44Q18BuYjAkF4EawWlmUU896PJXpBy8fI6ejMDlfQspwnfPZSUtt3TUMfEdTizbYB6UWCoaclcOB1Z9czKtqA+NZ+3SDaRTLwItRaw5O0QHjJIXgQoCSBeX6zST7Nu+5W7n6F8iBz8N5k04jBvYV+GFQ/COnD2aDUoZQPU5tXUAKH1IAtvSCc+AL5Y3zU8sK/iflD8LS6BQNDaqKBS8+7W0H1mj/0vFu27qfTqHBXYF+jBYf8/6vNibJoIhCLjNbHQQP4=
x-forefront-antispam-report:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY5PR21MB3759.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(7416005)(376005)(1800799015)(366007);DIR:OUT;SFP:1102;
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?Vc1rtwnwY2Ya1Ff4AYRnFLXccQQ8to5bxYkH5g8QVoI5YbBSTIP2FvUTuSZP?=
 =?us-ascii?Q?r2DrE3VSaJQ5plKVS9DJT6wQCGPRoTjLzXHg5RK0/fxC1Nl5iNsQ8mZ76xQP?=
 =?us-ascii?Q?yjOFv/9Ebr5Hi2ursitKz+AorrxZRTyy2DcHEOaWf1KlKnCv8U7thrAVxb8I?=
 =?us-ascii?Q?V5Q76ihRP2jEGSoU3w3NvMQnZzLmBagiINM3G7GYyKjOQhgS3bvcCowad5Z0?=
 =?us-ascii?Q?A6FH+/EMlEHf3BOEewTzR2mkuMe6qOduc3W64+/34xXNsNd02T8jaT0cnIg+?=
 =?us-ascii?Q?dSFXfZN9ryroOsmU/gLcG90LfbOCu2ySOgyAuZlUF0qUXok+w4Kvdh8mMjve?=
 =?us-ascii?Q?ZEb0/9Jwt4ITSl6yzYDNHLJaHaLx3BRoVjk2tFT+6paTwymv4VqBPIjSNc8L?=
 =?us-ascii?Q?sBLnEXnlVGpOWdRZBftuXNB0idTfundUsEs5e65p4FCTsYMZfTv1nJ82WX9e?=
 =?us-ascii?Q?Z/bJzqFbx0g2WmZSqd6AqPVXo34qyWDLPjeQgRa0TaDW8lpaMvWaS4zKFBmD?=
 =?us-ascii?Q?//QCe3hFBrGrXnLrAEIKf2dmp9T/7LJ+jgv9JW9jDxI4Zc4R2sfnqZ97keP6?=
 =?us-ascii?Q?GDfwqg8oa6p6NSEqRsKyVJ85u9+xnt5ykKZn+twZ2e67omXNjaThm/ewZ9WB?=
 =?us-ascii?Q?j12u6rnJtbONsAR1uyHgynhnEUD5BazcRZsbmNqvcXUMDsd68v+XLjT27wyl?=
 =?us-ascii?Q?N5lMx7dvY4JDTd94ukzRwAytc30/0VCh9FfI3HruO6gxhtemAHZ9NE5HU3BL?=
 =?us-ascii?Q?Ct6eG2I4+iiwlKpgh9kevHplk/OBTeYoAbblSg7UyiHRg6yJxxL7FfA2cdVa?=
 =?us-ascii?Q?Xu5dYSuU42GmO48hxJ5lCVzkGH0BafEUKRfNnik7sIxHNDbp3OS6RBoAXrlw?=
 =?us-ascii?Q?yS9rlHD2jmuQsa2MH/kOfHM8VUduYFsYvtkXhn89uBhsI5dIrw9Tb4jMG9UX?=
 =?us-ascii?Q?IRHNNYpIdtyWjG74M7XXJebHr/x83Pg/q7o2zg8HGQbnPr9t+kTCUGtoVWxB?=
 =?us-ascii?Q?zZxDkdch5mJhMGwyqu6Mprb4Qii1thLWDAFSEGRjdwkjJVf/BS5H61kGq6n8?=
 =?us-ascii?Q?RVsKCCvo9dV/AkRtL5Bbi8u9U7R3zK18LKZFKvY4nToyDFC3RSk4u/d2xdba?=
 =?us-ascii?Q?MmG+rH3uBl0wOGz4t9xrozJrfhYx3WlrFQ/cEFpnKEscbwpQD4ieg3QBjh9M?=
 =?us-ascii?Q?mFBoT097Ive6ZSTT4XXAkSIfGN56lComM8V3DcGbJhXUK5zh9iwZB9POVI11?=
 =?us-ascii?Q?+rXmMO4E9xpDjSqLyi0CJDEJRFb20BlePu/rT5XKwqATZzjcVDpRSeaPG8L5?=
 =?us-ascii?Q?ZNLcu3QnxpgMSYjaTe3C86s3B5ZqS4bZxCx1HOwURxC8qUMg7wVYAeJLva/B?=
 =?us-ascii?Q?TKYGkKhiQrVnzu3BvLlmn/0H75Ud43Ezj9/gCrbnx6T5pUvU5ldBtJ1qjeMU?=
 =?us-ascii?Q?g03qP5ANWZ0s1KQFJrf2ovAJn0qbKrVSd/BUOHbHWczQ8B1RmkSxRHQI0ikf?=
 =?us-ascii?Q?Yp+Wg37y9RcmKx3Aw27Gcww+TtXOfCA7dvQRj6uGuiDvS0YpCGSt0fhdc6Av?=
 =?us-ascii?Q?VURr6W4dT1ngXQzA5aZNjd2TvjxR8WPCfTNi9cW0VyDhRQT3MLgkRqpkkfS5?=
 =?us-ascii?Q?EZdrsyk2sxsHlWE/pXpeOLA=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: microsoft.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY5PR21MB3759.namprd21.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b685f989-80e9-4886-034d-08dc504e050a
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Mar 2024 00:12:00.1812
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 5Sk7dowv76SJFdpuaM3A5TvJZ5EvpwjeT1KF3NbYpnm024sjKOBjZ4oXDHbl3m3eV1nI6S4/I5eSs6vyOFBllg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR21MB1509

> From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang
> Sent: Friday, March 29, 2024 2:37 PM
> [...]
> mana_get_rxbuf_cfg() aligns the RX buffer's DMA datasize to be
> multiple of 64. So a packet slightly bigger than mtu+14, say 1536,
> can be received and cause skb_over_panic.
>=20
> Sample dmesg:
> [ 5325.237162] skbuff: skb_over_panic: text:ffffffffc043277a len:1536
> put:1536 head:ff1100018b517000 data:ff1100018b517100 tail:0x700
> end:0x6ea dev:<NULL>
> [ 5325.243689] ------------[ cut here ]------------
> [ 5325.245748] kernel BUG at net/core/skbuff.c:192!
> [ 5325.247838] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [ 5325.258374] RIP: 0010:skb_panic+0x4f/0x60
> [ 5325.302941] Call Trace:
> [ 5325.304389]  <IRQ>
> [ 5325.315794]  ? skb_panic+0x4f/0x60
> [ 5325.317457]  ? asm_exc_invalid_op+0x1f/0x30
> [ 5325.319490]  ? skb_panic+0x4f/0x60
> [ 5325.321161]  skb_put+0x4e/0x50
> [ 5325.322670]  mana_poll+0x6fa/0xb50 [mana]
> [ 5325.324578]  __napi_poll+0x33/0x1e0
> [ 5325.326328]  net_rx_action+0x12e/0x280
>=20
> As discussed internally, this alignment is not necessary. To fix
> this bug, remove it from the code. So oversized packets will be
> marked as CQE_RX_TRUNCATED by NIC, and dropped.
>=20
> Cc: stable@vger.kernel.org
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)")

While the unnecessary alignment indeed first appeared in ca9c54d2d6a5
(Apr 2021), ca9c54d2d6a5 didn't cause any crash because the only
supported MTU size was 1500, and the RX buffer was a page (which is
big enough to hold an incoming packet of a size up to 1536 bytes), and
mana_rx_skb() created a big skb of 1 page (which is big enough to
hold the incoming packet); the only issue with ca9c54d2d6a5 was that
an incoming packet of 1515~1536 bytes (if such a packet was ever
received by the NIC) was still properly delivered to the upper layer
network stack, while ideally such a packet should be dropped by the
NIC driver as we would have received CQE_RX_TRUNCATED if
ca9c54d2d6a5 hadn't done the unnecessary alignment.

So,  my understanding is that ca9c54d2d6a5 is imperfect
but it doesn't really cause any real issue.

It looks like the real issue started in the commit (Apr 12 14:16:02 2023)
2fbbd712baf1 ("net: mana: Enable RX path to handle various MTU sizes")
which introduces "rxq->alloc_size", which I think can be
smaller than rxq->datasize, and  is used in  mana_poll() --> ... ->
mana_build_skb(), which I think causes the crash because
the size of the allocated skb may not be able to hold a big
incoming packet.

Later, the commit (Apr 12 14:16:03 2023)
80f6215b450e ("net: mana: Add support for jumbo frame")
does code refactoring and introduces mana_get_rxbuf_cfg().

I suggest the Fixes tag should be updated to:
Fixes: 2fbbd712baf1 ("net: mana: Enable RX path to handle various MTU sizes=
")
, because if we used "Fixes: ca9c54d2d6a5", the distro vendors
would ask us: does this fix need to be backported to old kernels
that don't have 2fbbd712baf1? The fix can't apply cleanly there
and is not really needed there. The stable tree maintainers would
ask the same question.

>  drivers/net/ethernet/microsoft/mana/mana_en.c | 2 +-
>  include/net/mana/mana.h                       | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 59287c6e6cee..d8af5e7e15b4 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -601,7 +601,7 @@ static void mana_get_rxbuf_cfg(int mtu, u32
> *datasize, u32 *alloc_size,
>=20
>  	*alloc_size =3D mtu + MANA_RXBUF_PAD + *headroom;
>=20
> -	*datasize =3D ALIGN(mtu + ETH_HLEN, MANA_RX_DATA_ALIGN);
> +	*datasize =3D mtu + ETH_HLEN;
>  }
>=20

I suggest the Fixes tag should be updated. Otherwise the fix
looks good to me.

Reviewed-by: Dexuan Cui <decui@microsoft.com>