netdev - Re: [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB flags when picking the dst device

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf8f2ad4-c1d6-9a18-8555-353b526a783e@nvidia.com>
Date:   Mon, 2 Aug 2021 14:40:05 +0300
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Jiri Pirko <jiri@...nulli.us>, Ido Schimmel <idosch@...sch.org>,
        Roopa Prabhu <roopa@...dia.com>,
        bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net-next] net: bridge: switchdev: fix incorrect use of FDB
 flags when picking the dst device

On 02/08/2021 14:36, Vladimir Oltean wrote:
> Nikolay points out that it is incorrect to assume that it is impossible
> to have an fdb entry with fdb->dst == NULL and the BR_FDB_LOCAL bit in
> fdb->flags not set. This is because there are reader-side places that
> test_bit(BR_FDB_LOCAL, &fdb->flags) without the br->hash_lock, and if
> the updating of the FDB entry happens on another CPU, there are no
> memory barriers at writer or reader side which would ensure that the
> reader sees the updates to both fdb->flags and fdb->dst in the same
> order, i.e. the reader will not see an inconsistent FDB entry.
> 
> So we must be prepared to deal with FDB entries where fdb->dst and
> fdb->flags are in a potentially inconsistent state, and that means that
> fdb->dst == NULL should remain a condition to pick the net_device that
> we report to switchdev as being the bridge device, which is what the
> code did prior to the blamed patch.
> 
> Fixes: 52e4bec15546 ("net: bridge: switchdev: treat local FDBs the same as entries towards the bridge")
> Suggested-by: Nikolay Aleksandrov <nikolay@...dia.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  net/bridge/br_fdb.c       | 2 +-
>  net/bridge/br_switchdev.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 4ff8c67ac88f..af31cebfda94 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -745,7 +745,7 @@ static int br_fdb_replay_one(struct net_bridge *br, struct notifier_block *nb,
>  	item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>  	item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>  	item.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> -	item.info.dev = item.is_local ? br->dev : p->dev;
> +	item.info.dev = (!p || item.is_local) ? br->dev : p->dev;
>  	item.info.ctx = ctx;
>  
>  	err = nb->notifier_call(nb, action, &item);
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 023de0e958f1..36d75fd4a80c 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -134,7 +134,7 @@ br_switchdev_fdb_notify(struct net_bridge *br,
>  		.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags),
>  		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
>  	};
> -	struct net_device *dev = info.is_local ? br->dev : dst->dev;
> +	struct net_device *dev = (!dst || info.is_local) ? br->dev : dst->dev;
>  
>  	switch (type) {
>  	case RTM_DELNEIGH:
> 

Thanks,
Acked-by: Nikolay Aleksandrov <nikolay@...dia.com>