Page MenuHomeFreeBSD
Differential D59595

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

Authored by pouria on Fri, Sep 11, 6:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 1, 8:26 PM
Unknown Object (File)
Thu, Oct 1, 5:53 AM
Unknown Object (File)
Wed, Sep 30, 1:22 PM
Unknown Object (File)
Wed, Sep 30, 12:33 PM
Unknown Object (File)
Wed, Sep 30, 8:22 AM
Unknown Object (File)
Wed, Sep 30, 6:52 AM
Unknown Object (File)
Tue, Sep 29, 5:22 PM
Unknown Object (File)
Tue, Sep 29, 2:45 PM
View All 42 Files
Subscribers
imp

Details

Reviewers
glebius
markj
kfv
kib
Group Reviewers
network
Commits
rG287d60af1812: compat/linux: Fix IFLA_IFNAME translation for multi-message netlink
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 Not Applicable
Unit
Tests Not Applicable

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

glebius added a comment.Tue, Sep 15, 5:17 AM

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?

pouria updated this revision to Diff 186762.Tue, Sep 15, 9:16 AM
pouria marked an inline comment as done.

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
pouria added a comment.Wed, Sep 23, 7:07 PM

friendly ping @glebius

glebius accepted this revision as: glebius.Fri, Sep 25, 4:16 PM

Sorry for delay. IMHO, it is really not worth making perfect as this translation will soon go away.

This revision is now accepted and ready to land.Fri, Sep 25, 4:16 PM
Closed by commit rG287d60af1812: compat/linux: Fix IFLA_IFNAME translation for multi-message netlink (authored by pouria). · Explain WhyFri, Sep 25, 5:06 PM
This revision was automatically updated to reflect the committed changes.
pouria added a commit: rG287d60af1812: compat/linux: Fix IFLA_IFNAME translation for multi-message netlink.

Revision Contents
Changeset List

PathSize
sys/
compat/
linux/
13 lines

Diff 187694

View Options

sys/compat/linux/linux_netlink.c

Loading...