On Fri, Jul 9, 2021 at 5:35 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 9 Jul 2021 14:56:26 -0700, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 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. I suspect doing
> > this state check inside dev_get_by_index() would have unintended
> > consequences, though, right?
>
> In the function unregister_netdevice_many(), dev will be deleted from the linked
> list, so after this, dev_get_by_index() will not return dev. If it is not in
> rtnl_lock, subsequent use of dev is to check reg_state.
>
Ah, I see, makes sense, if we do dev lookup and attachment under the
same lock then we either won't get the device or at the time of
attachment it will be valid.
> So I think, maybe the version of v1 does not have the problem you mentioned.
> After calling rtnl_lock, we get dev from dev_get_by_index(). If it succeeds, we
> execute the following process, and if it fails, we return an error directly.
>
>
> >
> > 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?
>
> There is no such problem here, because all netlink operations are protected by
> rtnl_lock. In the protection of rtnl_lock, it is completely safe to get dev and
> attach link or prog.
>
Ok, I see, one big rtnl_lock saves netlink :)
>
> >
> > 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.
>
> As mentioned above, since the entire bpf prog operation is protected by
> rtnl_lock, dev_xdp_attach() does not need to check the status of dev.
>
> >
> > 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.
>
> I think I understand what you mean now.
Yeah, this is a problem regardless.
Btw, I was also thinking to move dev_get_by_index right before
dev_xdp_attach_link inside a tight rntl_lock/rtnl_unlock region after
bpf_link is allocated, but that seems pretty bad if user,
intentionally or not, passes wrong ifindex. We'll be allocated a bunch
of unnecessary memory and deferring freeing it for no good reason. So
let's go with your v1 and link->dev = NULL to cover the clean up bug.
Thanks!
>
> Thanks.
>
> >
> >
> > >
> > > > Reported-by: Abaci <abaci@xxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > Reviewed-by: Dust Li <dust.li@xxxxxxxxxxxxxxxxx>
> > >
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index c253c2aafe97..63c9a46ca853 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -9544,6 +9544,10 @@ static int dev_xdp_attach_link(struct net_device *dev,
> > > > struct netlink_ext_ack *extack,
> > > > struct bpf_xdp_link *link)
> > > > {
> > > > + /* ensure the dev state is ok */
> > > > + if (dev->reg_state != NETREG_REGISTERED)
> > > > + return -EINVAL;
> > > > +
> > > > return dev_xdp_attach(dev, extack, link, NULL, NULL, link->flags);
> > > > }
