Re: [PATCH net v2] ipv4: fix use-after-free in fib_nhc_update_mtu()

[Chengfeng Ye]

Dear Ido,

No problem, the patch looks really good, and I just posted it as v3
with a few minor comment wording changes. Thanks again for your
detailed reply.
https://lore.kernel.org/netdev/20260807181710.1178747-1-nicoyip.dev@xxxxxxxxx/T/#u

Best regards,
Chengfeng

On Thu, Aug 6, 2026 at 10:42 PM Ido Schimmel <idosch@xxxxxxxxxx> wrote:
>
> On Thu, Aug 06, 2026 at 09:33:09PM +0800, Chengfeng Ye wrote:
> > fib_nhc_update_mtu() walks the nexthop exception table under RTNL, but
> > RTNL does not serialize this walk with PMTU exception updates. The walk
> > uses rcu_dereference_protected() with a constant true condition without
> > holding fnhe_lock.
> >
> > The following interleaving can therefore occur:
> >
> >   CPU 0                              CPU 1
> >   fib_nhc_update_mtu()               update_or_create_fnhe()
> >     load fnhe                          spin_lock_bh(&fnhe_lock)
> >                                        fnhe_remove_oldest()
> >                                          unlink fnhe
> >                                          kfree_rcu(fnhe, rcu)
> >     <quiescent state>
> >     access fnhe after grace period
>
> [...]
>
> > The same walk updates fnhe_pmtu and fnhe_mtu_locked. These fields form a
> > pair and other writers serialize them with fnhe_lock. RCU alone would
> > prevent reclamation, but would still allow concurrent writers to leave a
> > mixed pair.
> >
> > Expose fnhe_lock to fib_semantics.c and hold it across the exception-table
> > walk. This prevents entries from being unlinked while they are visited and
> > serializes the paired PMTU state updates with all other writers.
>
> Looks correct, but can't we use RCU for the traversal and only acquire
> the global lock when updating an entry? Otherwise, whenever a device MTU
> changes, we acquire the global lock (and disable softIRQs) across a scan
> of 2048 buckets and we do that for each nexthop using this device.
>
> How about something like [1] (vibe coded, compile-tested only)?
>
> It also avoids exporting the global lock.
>
> Please wait at least 24h before posting v3.
>
> Thanks
>
> [1]
> diff --git a/include/net/route.h b/include/net/route.h
> index f90106f383c5..45290177a33c 100644
> --- a/include/net/route.h
> +++ b/include/net/route.h
> @@ -276,6 +276,8 @@ int fib_dump_info_fnhe(struct sk_buff *skb, struct netlink_callback *cb,
>                        u32 table_id, struct fib_info *fi,
>                        int *fa_index, int fa_start, unsigned int flags);
>
> +void fnhe_update_pmtu(struct fib_nh_exception *fnhe, u32 new, u32 orig);
> +
>  static inline void ip_rt_put(struct rtable *rt)
>  {
>         /* dst_release() accepts a NULL parameter.
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 4f3c0740dde9..61286948c5d9 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1864,42 +1864,30 @@ static int call_fib_nh_notifiers(struct fib_nh *nh,
>         return NOTIFY_DONE;
>  }
>
> -/* Update the PMTU of exceptions when:
> - * - the new MTU of the first hop becomes smaller than the PMTU
> - * - the old MTU was the same as the PMTU, and it limited discovery of
> - *   larger MTUs on the path. With that limit raised, we can now
> - *   discover larger MTUs
> - * A special case is locked exceptions, for which the PMTU is smaller
> - * than the minimal accepted PMTU:
> - * - if the new MTU is greater than the PMTU, don't make any change
> - * - otherwise, unlock and set PMTU
> +/* Walk the exceptions of a nexthop after its first hop MTU changed.  The
> + * chain is only RCU protected here, fnhe_update_pmtu() takes fnhe_lock for
> + * the update of each entry.
>   */
>  void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
>  {
>         struct fnhe_hash_bucket *bucket;
>         int i;
>
> -       bucket = rcu_dereference_protected(nhc->nhc_exceptions, 1);
> +       rcu_read_lock();
> +       bucket = rcu_dereference(nhc->nhc_exceptions);
>         if (!bucket)
> -               return;
> +               goto out;
>
>         for (i = 0; i < FNHE_HASH_SIZE; i++) {
>                 struct fib_nh_exception *fnhe;
>
> -               for (fnhe = rcu_dereference_protected(bucket[i].chain, 1);
> +               for (fnhe = rcu_dereference(bucket[i].chain);
>                      fnhe;
> -                    fnhe = rcu_dereference_protected(fnhe->fnhe_next, 1)) {
> -                       if (fnhe->fnhe_mtu_locked) {
> -                               if (new <= fnhe->fnhe_pmtu) {
> -                                       fnhe->fnhe_pmtu = new;
> -                                       fnhe->fnhe_mtu_locked = false;
> -                               }
> -                       } else if (new < fnhe->fnhe_pmtu ||
> -                                  orig == fnhe->fnhe_pmtu) {
> -                               fnhe->fnhe_pmtu = new;
> -                       }
> -               }
> +                    fnhe = rcu_dereference(fnhe->fnhe_next))
> +                       fnhe_update_pmtu(fnhe, new, orig);
>         }
> +out:
> +       rcu_read_unlock();
>  }
>
>  void fib_sync_mtu(struct net_device *dev, u32 orig_mtu)
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index fd688e1f879f..604cc51dfd9b 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -741,6 +741,35 @@ static void update_or_create_fnhe(struct fib_nh_common *nhc, __be32 daddr,
>         spin_unlock_bh(&fnhe_lock);
>  }
>
> +/* Update the PMTU of an exception when:
> + * - the new MTU of the first hop becomes smaller than the PMTU
> + * - the old MTU was the same as the PMTU, and it limited discovery of
> + *   larger MTUs on the path. With that limit raised, we can now
> + *   discover larger MTUs
> + * A special case is locked exceptions, for which the PMTU is smaller
> + * than the minimal accepted PMTU:
> + * - if the new MTU is greater than the PMTU, don't make any change
> + * - otherwise, unlock and set PMTU
> + *
> + * fnhe_lock keeps fnhe_pmtu and fnhe_mtu_locked consistent against
> + * update_or_create_fnhe(), which sets both under the same lock.
> + */
> +void fnhe_update_pmtu(struct fib_nh_exception *fnhe, u32 new, u32 orig)
> +{
> +       spin_lock_bh(&fnhe_lock);
> +
> +       if (fnhe->fnhe_mtu_locked) {
> +               if (new <= fnhe->fnhe_pmtu) {
> +                       fnhe->fnhe_pmtu = new;
> +                       fnhe->fnhe_mtu_locked = false;
> +               }
> +       } else if (new < fnhe->fnhe_pmtu || orig == fnhe->fnhe_pmtu) {
> +               fnhe->fnhe_pmtu = new;
> +       }
> +
> +       spin_unlock_bh(&fnhe_lock);
> +}
> +
>  static void __ip_do_redirect(struct rtable *rt, struct sk_buff *skb, struct flowi4 *fl4,
>                              bool kill_route)
>  {