Paths

Table of Contentst

compat/linux: fix IFLA_IFNAME translation for multi-message netlink
Needs ReviewPublic
Actions

Authored by pouria on Fri, Sep 11, 6:03 PM.

Details

Reviewers

glebius
markj
kfv
kib

Group Reviewers

network

Summary

nlmsg_translate_ifname_nla() always used nw->ifp for the name,
which is fine for a single-message ifnet event, but an RTM_GETLINK
dump holds one RTM_NEWLINK per interface and is translated with
the ifp the writer had when the buffer was flushed.

The root of the problem is that msgs_to_linux() takes a single
ifp for a buffer that may contain messages about many interfaces.

Use nw->ifp only when its index matches the ifi_index, otherwise
resolve the name by index.

Assisted by: Claude (Fable 5.1)

*Disclosure*:
I'm not an expert in compat/linux, but I know how our netlink works.
I used an *AI* assistant to analyze linux_netlink.c and draft the initial diff after I had narrowed the problem to the kernel side with strace.
I reviewed and reworked the patch and tested it myself (dump, single reply, up/down events and interface removal), and I take full responsibility for it.

Test Plan

On a machine with several interfaces enough for the dump to span more than one flush:

Dump: chroot /compat/ubuntu ip link show
Before:

# chroot /compat/ubuntu ip link sh                                                                                                                                             
1: epair0a: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                         
    link/ether 58:9c:fc:04:db:5c brd ff:ff:ff:ff:ff:ff          
2: epair0a: <LOOPBACK,MULTICAST,UP,LOWER_UP> mtu 16384 qdisc noqueue state UP qlen 1000                                         
    link/loopback                                                       
3: epair0a: <> mtu 1500 qdisc noqueue state DOWN qlen 1000      
    link/[247]                                                     
4: epair0a: <> mtu 1536 qdisc noqueue state DOWN qlen 1000                                                                      
    link/[244]                                                  
5: epair0a: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                         
    link/ether 58:9c:fc:10:31:1e brd ff:ff:ff:ff:ff:ff                                                                          
6: bridge0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                            
    link/ether 58:9c:fc:10:dc:2a brd ff:ff:ff:ff:ff:ff          
7: bridge0: <BROADCAST,MULTICAST,PROMISC,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                    
    link/ether 58:9c:fc:10:c0:8d brd ff:ff:ff:ff:ff:ff                                                                                                                                          
8: bridge0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                                                                                            
    link/ether 58:9c:fc:10:60:45 brd ff:ff:ff:ff:ff:ff                                                                          
9: bridge0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000                                         
    link/ether 58:9c:fc:10:e9:85 brd ff:ff:ff:ff:ff:ff

After:

# chroot /compat/ubuntu ip link sh
1: vtnet0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:04:db:5c brd ff:ff:ff:ff:ff:ff
2: lo: <LOOPBACK,MULTICAST,UP,LOWER_UP> mtu 16384 qdisc noqueue state UP qlen 1000
    link/loopback
3: pfsync0: <> mtu 1500 qdisc noqueue state DOWN qlen 1000
    link/[247]
4: enc0: <> mtu 1536 qdisc noqueue state DOWN qlen 1000
    link/[244]
5: epair0a: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:10:31:1e brd ff:ff:ff:ff:ff:ff
6: epair0b: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:10:dc:2a brd ff:ff:ff:ff:ff:ff
7: epair1a: <BROADCAST,MULTICAST,PROMISC,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:10:c0:8d brd ff:ff:ff:ff:ff:ff
8: epair1b: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:10:60:45 brd ff:ff:ff:ff:ff:ff
9: bridge0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP qlen 1000
    link/ether 58:9c:fc:10:e9:85 brd ff:ff:ff:ff:ff:ff

Single reply: chroot /compat/ubuntu ip link show dev vtnet0
Events: chroot /compat/ubuntu ip monitor link while running:
ifconfig epair0a down; ifconfig epair0a up

Removal: chroot /compat/ubuntu ip monitor link while running:
ifconfig epair0a destroy

Repeated 1 with compat.linux.use_real_ifnames=0 and 1

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76920
Build 73803: arc lint + arc unit

Event Timeline

pouria created this revision.Fri, Sep 11, 6:03 PM

pouria held this revision as a draft.

Herald added a subscriber: imp. · View Herald TranscriptFri, Sep 11, 6:03 PM

Harbormaster completed remote builds in B76781: Diff 186477.Fri, Sep 11, 6:03 PM

pouria published this revision for review.Fri, Sep 11, 6:14 PM

pouria edited the summary of this revision. (Show Details)

pouria edited the test plan for this revision. (Show Details)

pouria added inline comments.

sys/compat/linux/linux_netlink.c
256	The __DECONST part is ugly, but it was the only semi-clean way that I could think of

If you just remove the condition and always use ifname_bsd_to_linux_idx(ifinfo->ifi_index, ... wouldn't it be the same? Also, the __DECONST problem will go away.

P.S. We plan to remove all this eth0 translation in FreeBSD 17 completely, so our bar for how ugly is the change is lower than usually.

sys/compat/linux/linux_netlink.c
255	That's really dirty. Maybe just pass it as argument all the way from `rtnl_newlink_to_linux` to here?
256	I really can't understand why DECONST is needed? We are passing non-const argument to a function that takes const. What was the compilation error?

Use only idx for ifname lookup. @glebius done.

sys/compat/linux/linux_netlink.c
255	I prefer current approach myself, passing it would be fine too. But we already do these kind of pointer arithmetic at line 302 and since `nlmsg_copy_all_nla()` already structure natural and we call it with `sizeof(struct ifinfo)`, IMHO we shouldn't touch that considering `rtnl_newlink_to_linux` is not the only consumer of `nlmsg_copy_all_nla`. If you think passing ifinfo argument is better let me know and I'll change it.
256	AFAICU, the value of index won't change during this function call. So It's possible to only use idx, therefore I can remove the `if_getindex(__DECONST(if_t, nw->ifp))` part.

Harbormaster completed remote builds in B76920: Diff 186762.Tue, Sep 15, 9:17 AM

Revision Contents
Changeset List

Path

Size

sys/

compat/

linux/

linux_netlink.c

13 lines

Diff 186762

View Options

sys/compat/linux/linux_netlink.c

compat/linux: fix IFLA_IFNAME translation for multi-message netlinkNeeds ReviewPublicActions