All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Liang Jie <buaajxlj@163.com>
Cc: edumazet@google.com, davem@davemloft.net, pabeni@redhat.com,
	horms@kernel.org, anthony.l.nguyen@intel.com,
	andrew+netdev@lunn.ch, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Liang Jie <liangjie@lixiang.com>
Subject: Re: [PATCH net v3] net: Refine key_len calculations in rhashtable_params
Date: Fri, 27 Dec 2024 11:13:28 -0800	[thread overview]
Message-ID: <20241227111328.540ced11@kernel.org> (raw)
In-Reply-To: <20241220082436.1195276-1-buaajxlj@163.com>

On Fri, 20 Dec 2024 16:24:36 +0800 Liang Jie wrote:
> From: Liang Jie <liangjie@lixiang.com>
> 
> This patch improves the calculation of key_len in the rhashtable_params
> structures across the net driver modules by replacing hardcoded sizes
> and previous calculations with appropriate macros like sizeof_field()
> and offsetofend().
> 
> Previously, key_len was set using hardcoded sizes like sizeof(u32) or
> sizeof(unsigned long), or using offsetof() calculations. This patch
> replaces these with sizeof_field() and correct use of offsetofend(),
> making the code more robust, maintainable, and improving readability.
> 
> Using sizeof_field() and offsetofend() provides several advantages:
> - They explicitly specify the size of the field or the end offset of a
>   member being used as a key.
> - They ensure that the key_len is accurate even if the structs change in
>   the future.
> - They improve code readability by clearly indicating which fields are used
>   and how their sizes are determined, making the code easier to understand
>   and maintain.
> 
> For example, instead of:
>     .key_len    = sizeof(u32),
> we now use:
>     .key_len    = sizeof_field(struct mae_mport_desc, mport_id),
> 
> And instead of:
>     .key_len    = offsetof(struct efx_tc_encap_match, linkage),
> we now use:
>     .key_len    = offsetofend(struct efx_tc_encap_match, ip_tos_mask),
> 
> These changes eliminate the risk in certain scenarios of including
> unintended padding or extra data in the key, ensuring the rhashtable
> functions correctly.

IMHO the change is not worth the churn. Does any upstream code checker
/ tool prevent from new instances of this pattern occurring?
-- 
pw-bot: reject

      reply	other threads:[~2024-12-27 19:13 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-20  8:24 [PATCH net v3] net: Refine key_len calculations in rhashtable_params Liang Jie
2024-12-27 19:13 ` Jakub Kicinski [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241227111328.540ced11@kernel.org \
    --to=kuba@kernel.org \
    --cc=andrew+netdev@lunn.ch \
    --cc=anthony.l.nguyen@intel.com \
    --cc=buaajxlj@163.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=liangjie@lixiang.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.