net: bridge: use a stable FDB dst snapshot in RCU readers

Local FDB entries can be rewritten in place by `fdb_delete_local()`, which
updates `f->dst` to another port or to `NULL` while keeping the entry
alive. Several bridge RCU readers inspect `f->dst`, including
`br_fdb_fillbuf()` through the `brforward_read()` sysfs path.

These readers currently load `f->dst` multiple times and can therefore
observe inconsistent values across the check and later dereference.
In `br_fdb_fillbuf()`, this means a concurrent local-FDB update can change
`f->dst` after the NULL check and before the `port_no` dereference,
leading to a NULL-ptr-deref.

Fix this by taking a single `READ_ONCE()` snapshot of `f->dst` in each
affected RCU reader and using that snapshot for the rest of the access
sequence. Also publish the in-place `f->dst` updates in `fdb_delete_local()`
with `WRITE_ONCE()` so the readers and writer use matching access patterns.

Fixes: 960b589f86c7 ("bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address")
Cc: stable@kernel.org
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Zhengchuan Liang <zcliangcn@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Link: https://patch.msgid.link/6570fabb85ecadb8baaf019efe856f407711c7b9.1776043229.git.zcliangcn@gmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

authored by

Zhengchuan Liang and committed by

Paolo Abeni 2 months ago df460165 fa92a77b

+23 -13

2 changed files

expand all

net

bridge

br_arp_nd_proxy.c

br_fdb.c

+5 -3

net/bridge/br_arp_nd_proxy.c

··· 201 201 202 202 f = br_fdb_find_rcu(br, n->ha, vid); 203 203 if (f) { 204 + const struct net_bridge_port *dst = READ_ONCE(f->dst); 204 205 bool replied = false; 205 206 206 207 if ((p && (p->flags & BR_PROXYARP)) || 207 - (f->dst && (f->dst->flags & BR_PROXYARP_WIFI)) || 208 - br_is_neigh_suppress_enabled(f->dst, vid)) { 208 + (dst && (dst->flags & BR_PROXYARP_WIFI)) || 209 + br_is_neigh_suppress_enabled(dst, vid)) { 209 210 if (!vid) 210 211 br_arp_send(br, p, skb->dev, sip, tip, 211 212 sha, n->ha, sha, 0, 0); ··· 470 469 471 470 f = br_fdb_find_rcu(br, n->ha, vid); 472 471 if (f) { 472 + const struct net_bridge_port *dst = READ_ONCE(f->dst); 473 473 bool replied = false; 474 474 475 - if (br_is_neigh_suppress_enabled(f->dst, vid)) { 475 + if (br_is_neigh_suppress_enabled(dst, vid)) { 476 476 if (vid != 0) 477 477 br_nd_send(br, p, skb, n, 478 478 skb->vlan_proto,

+18 -10

net/bridge/br_fdb.c

··· 236 236 const unsigned char *addr, 237 237 __u16 vid) 238 238 { 239 + const struct net_bridge_port *dst; 239 240 struct net_bridge_fdb_entry *f; 240 241 struct net_device *dev = NULL; 241 242 struct net_bridge *br; ··· 249 248 br = netdev_priv(br_dev); 250 249 rcu_read_lock(); 251 250 f = br_fdb_find_rcu(br, addr, vid); 252 - if (f && f->dst) 253 - dev = f->dst->dev; 251 + if (f) { 252 + dst = READ_ONCE(f->dst); 253 + if (dst) 254 + dev = dst->dev; 255 + } 254 256 rcu_read_unlock(); 255 257 256 258 return dev; ··· 350 346 vg = nbp_vlan_group(op); 351 347 if (op != p && ether_addr_equal(op->dev->dev_addr, addr) && 352 348 (!vid || br_vlan_find(vg, vid))) { 353 - f->dst = op; 349 + WRITE_ONCE(f->dst, op); 354 350 clear_bit(BR_FDB_ADDED_BY_USER, &f->flags); 355 351 return; 356 352 } ··· 361 357 /* Maybe bridge device has same hw addr? */ 362 358 if (p && ether_addr_equal(br->dev->dev_addr, addr) && 363 359 (!vid || (v && br_vlan_should_use(v)))) { 364 - f->dst = NULL; 360 + WRITE_ONCE(f->dst, NULL); 365 361 clear_bit(BR_FDB_ADDED_BY_USER, &f->flags); 366 362 return; 367 363 } ··· 932 928 int br_fdb_fillbuf(struct net_bridge *br, void *buf, 933 929 unsigned long maxnum, unsigned long skip) 934 930 { 931 + const struct net_bridge_port *dst; 935 932 struct net_bridge_fdb_entry *f; 936 933 struct __fdb_entry *fe = buf; 937 934 unsigned long delta; ··· 949 944 continue; 950 945 951 946 /* ignore pseudo entry for local MAC address */ 952 - if (!f->dst) 947 + dst = READ_ONCE(f->dst); 948 + if (!dst) 953 949 continue; 954 950 955 951 if (skip) { ··· 962 956 memcpy(fe->mac_addr, f->key.addr.addr, ETH_ALEN); 963 957 964 958 /* due to ABI compat need to split into hi/lo */ 965 - fe->port_no = f->dst->port_no; 966 - fe->port_hi = f->dst->port_no >> 8; 959 + fe->port_no = dst->port_no; 960 + fe->port_hi = dst->port_no >> 8; 967 961 968 962 fe->is_local = test_bit(BR_FDB_LOCAL, &f->flags); 969 963 if (!test_bit(BR_FDB_STATIC, &f->flags)) { ··· 1089 1083 1090 1084 rcu_read_lock(); 1091 1085 hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) { 1086 + const struct net_bridge_port *dst = READ_ONCE(f->dst); 1087 + 1092 1088 if (*idx < ctx->fdb_idx) 1093 1089 goto skip; 1094 - if (filter_dev && (!f->dst || f->dst->dev != filter_dev)) { 1090 + if (filter_dev && (!dst || dst->dev != filter_dev)) { 1095 1091 if (filter_dev != dev) 1096 1092 goto skip; 1097 1093 /* !f->dst is a special case for bridge ··· 1101 1093 * Therefore need a little more filtering 1102 1094 * we only want to dump the !f->dst case 1103 1095 */ 1104 - if (f->dst) 1096 + if (dst) 1105 1097 goto skip; 1106 1098 } 1107 - if (!filter_dev && f->dst) 1099 + if (!filter_dev && dst) 1108 1100 goto skip; 1109 1101 1110 1102 err = fdb_fill_info(skb, br, f,

Configure Feed

Configure Feed