On Fri, Jul 9, 2021 at 5:20 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > On Fri, 9 Jul 2021 14:56:26 -0700 Andrii Nakryiko wrote: > > On Fri, Jul 9, 2021 at 12:43 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > > > > > On Fri, 9 Jul 2021 10:55:25 +0800 Xuan Zhuo wrote: > > > > The problem occurs between dev_get_by_index() and dev_xdp_attach_link(). > > > > At this point, dev_xdp_uninstall() is called. Then xdp link will not be > > > > detached automatically when dev is released. But link->dev already > > > > points to dev, when xdp link is released, dev will still be accessed, > > > > but dev has been released. > > > > > > > > dev_get_by_index() | > > > > link->dev = dev | > > > > | rtnl_lock() > > > > | unregister_netdevice_many() > > > > | dev_xdp_uninstall() > > > > | rtnl_unlock() > > > > rtnl_lock(); | > > > > dev_xdp_attach_link() | > > > > rtnl_unlock(); | > > > > | netdev_run_todo() // dev released > > > > bpf_xdp_link_release() | > > > > /* access dev. | > > > > use-after-free */ | > > > > > > > > This patch adds a check of dev->reg_state in dev_xdp_attach_link(). If > > > > dev has been called release, it will return -EINVAL. > > > > > > Please make sure to include a Fixes tag. > > > > > > I must say I prefer something closet to v1. Maybe put the if > > > in the callee? Making ndo calls to unregistered netdevs is > > > not legit, it will be confusing for a person reading this > > > code to have to search callees to find where unregistered > > > netdevs are rejected. > > > > So I'm a bit confused about the intended use of dev_get_by_index(). It > > doesn't seem to be checking that device is unregistered and happily > > returns dev with refcnt bumped even though device is going away. Is it > > the intention that every caller of dev_get_by_index() needs to check > > the state of the device *and* do any subsequent actions under the same > > rtnl_lock/rtnl_unlock region? Seems a bit fragile. > > It depends on the caller, right? Not all callers even take the rtnl > lock. AFAIU dev_get_by_index() gives the caller a ref'ed netdev object. > If all the caller cares about is the netdev state itself that's > perfectly fine. > > If caller has ordering requirements or needs to talk to the driver > chances are the lookup and all checks should be done under rtnl. > Or there must be some lock dependency on rtnl (take a lock which > unregister netdev of the device of interest would also take). > > In case of XDP we impose extra requirements on ourselves because we > want the driver code to be as simple as possible. > > > I suspect doing this state check inside dev_get_by_index() would have > > unintended consequences, though, right? > > It'd be moot, dev_get_by_index() is under RCU and unregister path syncs > RCU, but that doesn't guarantee anything if caller holds no locks. Yep. As Xuan also mentioned, if dev_get_by_index and attach happens under the same lock then we can't really get dev that's unregistered. Ok, all makes sense, thanks for explaining. > > > BTW, seems like netlink code doesn't check the state of the device and > > will report successful attachment to the dev that's unregistered? Is > > this something we should fix as well? > > Entire rtnetlink is under rtnl_lock, and so is unregistering a netdev > so those paths can't race. > > > Xuan, if we do go with this approach, that dev->reg_state check should > > probably be done in dev_xdp_attach() instead, which is called for both > > bpf_link-based and bpf_prog-based XDP attachment. > > > > If not, then the cleanest solution would be to make this check right > > before dev_xdp_attach_link (though it's not clear what are we gaining > > with that, if we ever have another user of dev_xdp_attach_link beside > > bpf_xdp_link_attach, we'll probably miss similar situation), instead > > of spreading out rtnl_unlock. > > > > BTW, regardless of the approach, we still need to do link->dev = NULL > > if dev_xdp_attach_link() errors out.
