Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   Suman Ghosh <sumang@marvell.com>
To:     Shigeru Yoshida <syoshida@redhat.com>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "dsahern@kernel.org" <dsahern@kernel.org>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [EXT] [PATCH net v2] ipv4: ip_gre: Avoid skb_pull() failure in
 ipgre_xmit()
Thread-Topic: [EXT] [PATCH net v2] ipv4: ip_gre: Avoid skb_pull() failure in
 ipgre_xmit()
Thread-Index: AQHaJTq+xdxLhbQE0EyCVUEj9YjahLCXIVzggABTB4CAADd/wA==
Date:   Sun, 3 Dec 2023 15:17:09 +0000
Message-ID: <SJ0PR18MB5216005BB27035DE7C37CAA4DB87A@SJ0PR18MB5216.namprd18.prod.outlook.com>
References: <20231202161441.221135-1-syoshida@redhat.com>
        <SJ0PR18MB5216A25BD74AE376FB1E536BDB87A@SJ0PR18MB5216.namprd18.prod.outlook.com>
 <20231203.205409.646024453190363727.syoshida@redhat.com>
In-Reply-To: <20231203.205409.646024453190363727.syoshida@redhat.com>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jcB85JkAuhwIG99YwhuIeNxO8es9PZ3xQAF55O5lndHAPItiYKQD3CSEcc5g?=
 =?us-ascii?Q?viUXhWhgQLXbZVUCjSoquHC2S5tXo1wUtNOivmx2Sb1V0sB1n8lOy4OzADZG?=
 =?us-ascii?Q?VNa2QWKJ5Vapp/AX9TqeFXEinQ9kjA2Xl1THqsTcl42v5cX4lQ2vQf8irwNu?=
 =?us-ascii?Q?InWvXOjL3I572ndldYk6eMFMiQdNcT/eSxeA4ELR/DyqxaN1LZr0xCcDX/+a?=
 =?us-ascii?Q?KpgitqAzujfA4JvqHe7a3WTEZwI4iymUFJFom6f65tYgTnoSe3tIbGYF+hlh?=
 =?us-ascii?Q?xz8tAgbnlYS+LuvYV3Z6kUiJkzbQylSU4R0ckSmbVv3F9VBDb68D4PyK86cJ?=
 =?us-ascii?Q?5wC11Pg5WnfbHvwgzItk9gcb9R73/ZyBSeJCssOKPBa0rrJbb8hJyvJ9kH+F?=
 =?us-ascii?Q?sk1dSkeM2I0N7aAxjymRHKwJOflRqA4q1slgdGgKf5SvcQq6Efw/wrZa5KsG?=
 =?us-ascii?Q?/X+TsschS9oICHebPPPx2qk5m3e76s/vlqU8x74kHJoBfIAz/9TzPdAYsaqt?=
 =?us-ascii?Q?Fd64FvR+Ct02qNQB4r3ND7O1dt5SB+BAemEnQCBNQW5pn+rihhztixT58gHG?=
 =?us-ascii?Q?LlC0LFlBeSzV425eJep4k5Ea7V307rjUligWZH0K9RnA4xi+YUez11hZfluC?=
 =?us-ascii?Q?r1KB8BhWzkgBsqUe//BGLORVojpr5SBQyOJa9hY1MA4N2Lbi0ZCOn4y+IGDy?=
 =?us-ascii?Q?sRPsCMROLZphQ9ew1LuMbd2H5+9wf512MBBwIvOmweA1N75y0FDk/5+ahbJe?=
 =?us-ascii?Q?2CKSWyjmPbXH0n7Dw8MEYOUqxBItKt9Gc7iVPY/QxLCV3if+Qb0LeNEI7kj3?=
 =?us-ascii?Q?z4X39v34BZ1hkfQ1E4KvcdSzZv0VtdUZEE3VEwYnubSk8zsXrgZjd1BmuCfn?=
 =?us-ascii?Q?h9H0k0mmiyuehMsf3FlgtMgFoMJyqWdbIlMIqJuKEDAv9LUM8UlML3v0p8jF?=
 =?us-ascii?Q?wGglP3Hr9f2ayjn4id7cLnDuSNdp7oE6BJtpp5cQ480yhSAhYHETWlFw49F3?=
 =?us-ascii?Q?d3MNtVlIcH60PBS9FXJkht6yQ3amCzWjMcpiz3y/GETMpVo0meykSNXv5Wx6?=
 =?us-ascii?Q?YmffSBDUv0ltX0i1j8j8xjCMeT33qMRo0WHIvBq7Q/4eSNksoXyuIhcPxU/1?=
 =?us-ascii?Q?5XBbr05SQc8m0PHqf0X9O8iGL7lQZXKR2SkeSc9Y4h02dVUpZ0RK2NxR+EH9?=
 =?us-ascii?Q?ZrAscr/XA4+IDDobF62Ym5WEjoXPYvHISEpYOi4ZUNSg1B6TEK5DNUeIi0tm?=
 =?us-ascii?Q?SoSBowygyOlD4OFNCNAQzsBg4TjNmpygNo0zlD9wSFTh0RSJIIK26kZQxThS?=
 =?us-ascii?Q?bep5GYMBcPHSJmx7YT5cGrQH8WsjZhFpyeozjF9SVBNyDbSqmmRBN1W//zjT?=
 =?us-ascii?Q?WdY92RbG6CLPcy1J9ELDCfVeYHJNut5ka8DNXvzZiNvGWHqYU32GqLlG1xX6?=
 =?us-ascii?Q?G50hj1po4/Co/L8zUGgOGsp8nsA2fav5RmKpdwMvSYFPoZj1+zRpHCgaT4y0?=
 =?us-ascii?Q?uKItUckywaAlNTiKk2WIspINJQZdm/7xLfMkdkjvlCPkcoAmryxWEqbSrjS0?=
 =?us-ascii?Q?AQcnL+6E9m+sYp+5mEFUgRizwmtUQWnKSx40gBlB?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR18MB5216.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 865a9385-3f34-4199-e4c2-08dbf412eb1c
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Dec 2023 15:17:09.3720
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MYluCN6Yx6tjp9pNNa2jybeVDTK6368hKzd6OnChrh6xBAFOGEmN5ETq6/BH7/9ShksYWHr07rFeh4UdDyHrQA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR18MB4422
X-Proofpoint-ORIG-GUID: aWFmc8dPX5TfKjr6rByLDXRCdC4V-1xm
X-Proofpoint-GUID: aWFmc8dPX5TfKjr6rByLDXRCdC4V-1xm
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-03_13,2023-11-30_01,2023-05-22_02
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,
	SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sun, 03 Dec 2023 07:17:51 -0800 (PST)

>>> 	}
>>>
>>> 	if (dev->header_ops) {
>>>+		int pull_len =3D tunnel->hlen + sizeof(struct iphdr);
>>>+
>>> 		if (skb_cow_head(skb, 0))
>>> 			goto free_skb;
>>>
>>> 		tnl_params =3D (const struct iphdr *)skb->data;
>>>
>>>-		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>>>-		 * to gre header.
>>>-		 */
>>>-		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
>>>+		if (!pskb_network_may_pull(skb, pull_len))
>> [Suman] Since this is transmit path, should we add unlikely() here?
>
>Thanks for your comment.
>
>I traced this function and found that pskb_may_pull_reason() seems to
>have appropriate likely() and unlikely() as Eric says.
>
>I'm new to Linux networking. Could you kindly explain the background of
>your suggestion?
>
>I understand that a transmit path must be as fast as possible, so we
>should use unlikely() for rare cases such like this error path. Am I
>correct?
>
>Thanks,
>Shigeru
[Suman] Yes. Likely()/unlikely() helps the compiler for branch prediction a=
nd we use it mostly on the data path.
But I cross checked that this is static inline and the function pskb_may_pu=
ll() already have likely()/unlikely() in place.
So, you can ignore my comment here.
>
>>>+			goto free_skb;
>>>+
>>>+		/* ip_tunnel_xmit() needs skb->data pointing to gre header. */
>>>+		skb_pull(skb, pull_len);
>>> 		skb_reset_mac_header(skb);
>>>
>>> 		if (skb->ip_summed =3D=3D CHECKSUM_PARTIAL &&
>>>--
>>>2.41.0
>>>
>>