Received-SPF: pass (google.com: domain of linux-kernel+bounces-172958-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <90fd7cd7-72dc-4df6-88ec-fbc8b64735ad@intel.com>
Date: Wed, 8 May 2024 11:10:46 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] netdevice: define and allocate &net_device
 _properly_
To: Eric Dumazet <edumazet@google.com>
CC: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>,
	Paolo Abeni <pabeni@redhat.com>, Kees Cook <keescook@chromium.org>, "Gustavo
 A. R. Silva" <gustavoars@kernel.org>, Simon Horman <horms@kernel.org>,
	<nex.sw.ncis.osdt.itp.upstreaming@intel.com>,
	<linux-hardening@vger.kernel.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
References: <20240507123937.15364-1-aleksander.lobakin@intel.com>
 <CANn89iLANEp-tjkKSawPTmH8DxaSQZ_OoJaAYHjLPkmwGEJ6nw@mail.gmail.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <CANn89iLANEp-tjkKSawPTmH8DxaSQZ_OoJaAYHjLPkmwGEJ6nw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V2JaT3dRMllaMmUydVVPNlFBaEVlUFdqMVNaR0NhYkJmR21BK0tWWFdEV3Qr?=
 =?utf-8?B?NUd1UHRBbThmRHlYSWsxbFVGM0JLbzRLZHViYlgvQUZoaUNyOUtJdkR4UXdV?=
 =?utf-8?B?WnJIYnIyejNLcWVBNzZVQ0pybDFzSWd6eUpsbGlPQjhSWlBtNHNPTWdzTlVv?=
 =?utf-8?B?TnJONzVuTVorNE9yZE9FOGpjTmhlYzJpblhqaEJoTjAvV0NiaXpSNkxkdWto?=
 =?utf-8?B?NU9acURLSDFFaEZ5U1dJOVNhYmpQY2VYdUF0bkwzd2htN2tKMk9XbCtvWDBv?=
 =?utf-8?B?T1VwNktnTytxdUJraDNMSjJvdTRXOVFwYW1VSDZqUi9MUWdlcy9zV2xROVZX?=
 =?utf-8?B?V2xBZ2l0S0p1YWkvTEJLTFJiWmE2ZTVWK1FhZ042REtxTVBEOG4yNStLOGsr?=
 =?utf-8?B?RXZ1bHBUK0xLUHRzaFlkTUZDZ2lnQTg3L1JtQkd5aWhUeWtNdVJMaTA4VHFu?=
 =?utf-8?B?VU1jRUJrZWNpenRxWVpQM05PUEdaT2x5enFHVE8xUHVHNHZTSE05OTR4YTlH?=
 =?utf-8?B?N2J0RFluam5wSkh2R1I5R3A3MzFvNlI5SXBvZE1ic3Z6M2paeCtFSGlTSG0y?=
 =?utf-8?B?a1FwWm9xbmd5MldpT1NvaWNYUEowTEdPQnFuVHNLdHdSYVAyMHo4RllNc00y?=
 =?utf-8?B?QUhNZ2ZLMDVlUitML3ozQXZuQ0NIOFoxK2k3Q29NRWxyeTg4M09kdjFLeSs2?=
 =?utf-8?B?MjhYck1kbUZLYUN0TVdEZmtablM4ZnRHK3NFbTREY3d5SHRKOW9EaWpxeSt2?=
 =?utf-8?B?MnBUM3V6TTdUZWRtUHZmT3lPVEJBQXcwU3NSUHM4Z3lwVWdWejZJSlFYN1RT?=
 =?utf-8?B?ZHBDamNFQ2tVTmloSEVramF6WlFHa0J2VkNrQ3NxaXdRMHNJSWJDK0JvRUNI?=
 =?utf-8?B?SHlQTVZIanA3MTBDSkxwTTFWR2lub05uVXV2YW9SZ3o4L3Q2ZTNVb2RQQ00r?=
 =?utf-8?B?T1hHazBUNmlRRUVIbzNOdE5YSHVWVTJES1BXWXExNEZIN2p5SHdWMmJjam5t?=
 =?utf-8?B?blJMMHU3ZW9BL25HU0xIVzZoYjhhMmJ0Q2k5dEtLNE5raXJKNVVVeW9hMUxH?=
 =?utf-8?B?RnJTcC9TRXBVMHMrQitPTVBxUzB2bEhuZzFvOXJONk50NndhTFZzeXZQNWM1?=
 =?utf-8?B?d2ZwVng3YWFsdko2OGxaenhyMjVSWWErNGl2dVJkYjYyaGdXb1dOaTM2MWgr?=
 =?utf-8?B?S2VTU3A3cWZGbHMwSzRMSCtucjNQY0F4c29SM2pkMklRMHNuVEphcEROSmMx?=
 =?utf-8?B?amg0VUxuRWEvRzJUWTg5ZjhuTWh1RFJxVHk4MlVvSnFqb0RzZmZzazFGUVZY?=
 =?utf-8?B?SzJkZGIvNkVDaSthTERublVycWpIQkN4ZkhlalZzb3JSYXpDd1lUTElnbFZY?=
 =?utf-8?B?Y3ZXYjdQWkVwQU5WMmNrTkRyc3YzandBWWZTQzlyTERENnBGOVR6WVhUazZL?=
 =?utf-8?B?bHJDN3FLQVYwbVl4S0xNck9hSVE4WGZrdXZ1cmZpUitKc242ZzhBL2lnbDVw?=
 =?utf-8?B?eEdNY3VwRE4xVi9LS2o1cFNnekJTUG1JNG9VbHB5bWYyNldmNXNsdUFpeXNY?=
 =?utf-8?B?UnBGSkhlRHF3bGpTZnJ4cndVM09xTU5DY1YwUlZBblcxRCs0Q09YN3QxODNu?=
 =?utf-8?B?SDFGdDZBTDNuOUpsNXo1cElzNHk3elB5VDBFR2JFTFJCZjhxWnJIT2V0Qnda?=
 =?utf-8?B?aStRNE8xZGR4UzI2ZUEyYmRSdllPUVdtcGcyRFh5Skt2T2QzbXNaNllGUFVw?=
 =?utf-8?B?R1BUTk9Wd1RpdnlBTnlzUUlLb3lqd2xma1BBK0F1aGhBTExTN0FKRXhIU1Yz?=
 =?utf-8?B?ZEo1R1oxaGEydHBkMVFoOXF2Q2Fha0RWYmJmUXhTLzdRS2x6NEtucVg1M3k2?=
 =?utf-8?B?NTV2VUhnQmI1ek82NmtOWTRCRVBuejJFTDhHVmZXdnBlTnVVVkUxNkN1dHlQ?=
 =?utf-8?B?dENaTFkrZXpQUldmV0VTWStiL3IzaUQveExWaURpb0RGRzhZVHpBMGxqb3RR?=
 =?utf-8?B?bkVnM0NZL2VvRElPdWlSMVR1NnVYNUZkVWcwdVRhcW5iUGcwenZXT29ObnVJ?=
 =?utf-8?B?QjhtQ3J5SWwvUmR4STRNQ3NlZ3U1R0pPTUlZVGc0WHNHTFppUldROTVTVkRh?=
 =?utf-8?B?Rjc4MmpRSlUvZk5DMXhzUFFDNHRXQ2U4eWpOUS9kalpEMUx2VkY1dTVpS3ow?=
 =?utf-8?B?RVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c73d623-b1bc-4dc4-4701-08dc6f3ed02b
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 May 2024 09:11:15.3171
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1L+wmnOesKJ41Ld/maM64caiYZJForZcfSqlebDdpirEP4DY6tf5k9eptDDFzxTcehZOOhxYE6/wNRzE4DBcEJCgyp1lsg3wIrxHTvUYcOU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7421
X-OriginatorOrg: intel.com

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 7 May 2024 20:21:58 +0200

> On Tue, May 7, 2024 at 2:40 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> In fact, this structure contains a flexible array at the end, but
>> historically its size, alignment etc., is calculated manually.
>> There are several instances of the structure embedded into other
>> structures, but also there's ongoing effort to remove them and we
>> could in the meantime declare &net_device properly.
>> Declare the array explicitly, use struct_size() and store the array
>> size inside the structure, so that __counted_by() can be applied.
>> Don't use PTR_ALIGN(), as SLUB itself tries its best to ensure the
>> allocated buffer is aligned to what the user expects.
>> Also, change its alignment from %NETDEV_ALIGN to the cacheline size
>> as per several suggestions on the netdev ML.
>>
>> bloat-o-meter for vmlinux:
>>
>> free_netdev                                  445     440      -5
>> netdev_freemem                                24       -     -24
>> alloc_netdev_mqs                            1481    1450     -31
>>
>> On x86_64 with several NICs of different vendors, I was never able to
>> get a &net_device pointer not aligned to the cacheline size after the
>> change.
>>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
>> ---
> 
> ...
> 
>> -       p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
>> -       if (!p)
>> +       sizeof_priv = ALIGN(sizeof_priv, SMP_CACHE_BYTES);
> 
> If we have a __counted_by(priv_len), why do you ALIGN(sizeof_priv,
> SMP_CACHE_BYTES) ?

To have the whole block of &net_device + private part aligned to a
cacheline size.

> 
> If a driver pretends its private part is 4 bytes, we should get a
> warning if 20 bytes are used instead.

Ah okay, so this should be

	p = kvzalloc(struct_size(ALIGN(sizeof_priv, SMP_CACHE_BYTES)));
	p->priv_len = sizeof_priv;

> 
> You added two ____cacheline_aligned already in net_device already.

The whole size passed to kvzalloc() must be cacheline-aligned, otherwise
the MM layer can miscalculate the pointer alignment.

Thanks,
Olek