Re: [PATCH net-next v10 04/14] net: hold netdev instance lock during qdisc ndo_setup_tc

[Stanislav Fomichev <stfomichev@gmail.com>]

On 03/13, Eric Dumazet wrote:
> On Wed, Mar 5, 2025 at 5:37 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Qdisc operations that can lead to ndo_setup_tc might need
> > to have an instance lock. Add netdev_lock_ops/netdev_unlock_ops
> > invocations for all psched_rtnl_msg_handlers operations.
> >
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: Cong Wang <xiyou.wangcong@gmail.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>
> > Cc: Saeed Mahameed <saeed@kernel.org>
> > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  net/sched/sch_api.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index 21940f3ae66f..f5101c2ffc66 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -1279,9 +1279,11 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> >                          * We replay the request because the device may
> >                          * go away in the mean time.
> >                          */
> > +                       netdev_unlock_ops(dev);
> >                         rtnl_unlock();
> >                         request_module(NET_SCH_ALIAS_PREFIX "%s", name);
> >                         rtnl_lock();
> 
> Oops, dev might have disappeared.
> 
> As explained a few lines above in the comment :
> 
> /* We dropped the RTNL semaphore in order to
> * perform the module load.  So, even if we
> * succeeded in loading the module we have to
> * tell the caller to replay the request.  We
> * indicate this using -EAGAIN.
> * We replay the request because the device may
> * go away in the mean time.
> */
> 
> 
> 
> > +                       netdev_lock_ops(dev);
> 
> So this might trigger an UAF.

Oh, good catch, let me try to add more logic here to be more careful
on the replay path. We can make the caller not unlock the instance lock
in this case I think..