| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260206040251.292605-1-kuniyu@google.com>
Date: Fri, 6 Feb 2026 03:57:54 +0000
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: kota.toda@...-cybersecurity.com
Cc: edumazet@...gle.com, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
pabeni@...hat.com, yuki.koike@...-cybersecurity.com, kuniyu@...gle.com
Subject: Re: [PATCH net 1/2] net: bonding: fix type-confusion in bonding header_ops
Hi,
Looks like patches were copy-and-pasted to email client since
diff is corrupted (\t is replaced with \s, line is wrapped, etc).
You may want to check your email client config or simply use
git send-email like
$ git send-email --annotate --cover-letter --thread --no-chain-reply-to \
--subject-prefix "PATCH v3 net" \
--to "NAME <EMAIL@...RESS>" \
--cc "NAME <EMAIL@...RESS>" \
--cc netdev@...r.kernel.org \
--dry-run HEAD~2
Then, make sure to CC maintainers listed by ./scripts/get_maintainer.pl
There are a few more rules to follow, so please read these doc
for next submission.
Documentation/process/submitting-patches.rst
Documentation/process/maintainer-netdev.rst
From: 戸田晃太 <kota.toda@...-cybersecurity.com>
Date: Thu, 5 Feb 2026 19:57:00 +0900
> Add two members to struct bonding, bond_header_ops and
> header_slave_dev, to avoid type-confusion while keeping track of the
> slave's header_ops.
>
> Fixes: 1284cd3a2b740 (bonding: two small fixes for IPoIB support)
> Signed-off-by: Kota Toda <kota.toda@...-cybersecurity.com>
> Signed-off-by: Yuki Koike <yuki.koike@...-cybersecurity.com>
> Co-developed-by: Yuki Koike <yuki.koike@...-cybersecurity.com>
> Reviewed-by: Paolo Abeni <pabeni@...hat.com>
> Reported-by: Kota Toda <kota.toda@...-cybersecurity.com>
nit: Reported-by is not needed when it's identical with the author.
> ---
> drivers/net/bonding/bond_main.c | 66 ++++++++++++++++++++++++++++++++-
> include/net/bonding.h | 5 +++
> 2 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f17a170d1..5ecc64e38 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1616,14 +1616,70 @@ static void bond_compute_features(struct bonding *bond)
> netdev_change_features(bond_dev);
> }
>
> +static int bond_hard_header(struct sk_buff *skb, struct net_device *dev,
> + unsigned short type, const void *daddr,
> + const void *saddr, unsigned int len)
> +{
> + struct bonding *bond = netdev_priv(dev);
> + struct net_device *slave_dev;
> +
> + slave_dev = bond->header_slave_dev;
> +
> + return dev_hard_header(skb, slave_dev, type, daddr, saddr, len);
> +}
> +
> +static void bond_header_cache_update(struct hh_cache *hh, const
> +struct net_device *dev,
> + const unsigned char *haddr)
> +{
> + const struct bonding *bond = netdev_priv(dev);
> + void (*cache_update)(struct hh_cache *hh,
> + const struct net_device *dev,
> + const unsigned char *haddr);
> + struct net_device *slave_dev;
> +
> + slave_dev = bond->header_slave_dev;
> +
> + if (!slave_dev->header_ops
Is slave_dev always non-NULL ?
> || !(cache_update =
> READ_ONCE(slave_dev->header_ops->cache_update)))
READ_ONCE() seems necessary for header_ops.
and please run ./scripts/checkpatch.pl, which will
complain about the coding style setting var in if().
> + return;
> +
> + cache_update(hh, slave_dev, haddr);
> +}
> +
> +
> static void bond_setup_by_slave(struct net_device *bond_dev,
> struct net_device *slave_dev)
> {
> + struct bonding *bond = netdev_priv(bond_dev);
> bool was_up = !!(bond_dev->flags & IFF_UP);
>
> dev_close(bond_dev);
>
> - bond_dev->header_ops = slave_dev->header_ops;
> + /* Some functions are given dev as an argument
> + * while others not. When dev is not given, we cannot
> + * find out what is the slave device through struct bonding
> + * (the private data of bond_dev). Therefore, we need a raw
> + * header_ops variable instead of its pointer to const header_ops
> + * and assign slave's functions directly.
> + * For the other case, we set the wrapper functions that pass
> + * slave_dev to the wrapped functions.
> + */
> + bond->bond_header_ops.create = bond_hard_header;
> + bond->bond_header_ops.cache_update = bond_header_cache_update;
> + if (slave_dev->header_ops) {
> + WRITE_ONCE(bond->bond_header_ops.parse, slave_dev->header_ops->parse);
Are WRITE_ONCE()s needed ?
RCU readers will see NULL bond_dev->header_ops at this
point, no ?
If they see non-NULL header_ops, then it will not be changed
because only bond_release_and_destroy() clears it and
we wait one RCU grace period after issuing NETDEV_UNREGISTER.
> + WRITE_ONCE(bond->bond_header_ops.cache, slave_dev->header_ops->cache);
> + WRITE_ONCE(bond->bond_header_ops.validate,
> slave_dev->header_ops->validate);
> + WRITE_ONCE(bond->bond_header_ops.parse_protocol,
> slave_dev->header_ops->parse_protocol);
> + } else {
> + WRITE_ONCE(bond->bond_header_ops.parse, NULL);
> + WRITE_ONCE(bond->bond_header_ops.cache, NULL);
> + WRITE_ONCE(bond->bond_header_ops.validate, NULL);
> + WRITE_ONCE(bond->bond_header_ops.parse_protocol, NULL);
> + }
> +
> + bond->header_slave_dev = slave_dev;
> + bond_dev->header_ops = &bond->bond_header_ops;
Rather WRITE_ONCE() seems necessary here.
>
> bond_dev->type = slave_dev->type;
> bond_dev->hard_header_len = slave_dev->hard_header_len;
> @@ -2682,6 +2738,14 @@ static int bond_release_and_destroy(struct
> net_device *bond_dev,
> struct bonding *bond = netdev_priv(bond_dev);
> int ret;
>
> + /* If slave_dev is the earliest registered one, we must clear
> + * the variables related to header_ops to avoid dangling pointer.
> + */
> + if (bond->header_slave_dev == slave_dev) {
> + bond->header_slave_dev = NULL;
> + bond_dev->header_ops = NULL;
Ditto.
Also, before clearing header_ops, there would be a small (race)
window where
bond->header_slave_dev == NULL &&
bond_dev->header_ops == &bond->bond_header_ops
is true and this would trigger null-deref in bond_header_cache_update().
This is Eric's point, I think.
> + }
> +
> ret = __bond_release_one(bond_dev, slave_dev, false, true);
> if (ret == 0 && !bond_has_slaves(bond) &&
> bond_dev->reg_state != NETREG_UNREGISTERING) {
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 95f67b308..c37800e17 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -215,6 +215,11 @@ struct bond_ipsec {
> */
> struct bonding {
> struct net_device *dev; /* first - useful for panic debug */
> + struct net_device *header_slave_dev; /* slave net_device for
> header_ops */
> + /* maintained as a non-const variable
> + * because bond's header_ops should change depending on slaves.
> + */
> + struct header_ops bond_header_ops;
> struct slave __rcu *curr_active_slave;
> struct slave __rcu *current_arp_slave;
> struct slave __rcu *primary_slave;
> --
Powered by blists - more mailing lists