Re: [PATCH V2] netrom: Prevent race conditions between multiple add route

Lizhi Xu
2025-10-21T08:34:32Z

On Tue, 21 Oct 2025 09:36:47 +0300, Dan Carpenter wrote:
> On Tue, Oct 21, 2025 at 10:05:33AM +0800, Lizhi Xu wrote:
> > On Mon, 20 Oct 2025 20:59:24 +0300, Dan Carpenter wrote:
> > > On Mon, Oct 20, 2025 at 09:49:12PM +0800, Lizhi Xu wrote:
> > > > On Mon, 20 Oct 2025 21:34:56 +0800, Lizhi Xu wrote:
> > > > > > Task0 Task1 Task2
> > > > > > ===== ===== =====
> > > > > > [97] nr_add_node()
> > > > > > [113] nr_neigh_get_dev() [97] nr_add_node()
> > > > > > [214] nr_node_lock()
> > > > > > [245] nr_node->routes[2].neighbour->count--
> > > > > > [246] nr_neigh_put(nr_node->routes[2].neighbour);
> > > > > > [248] nr_remove_neigh(nr_node->routes[2].neighbour)
> > > > > > [283] nr_node_unlock()
> > > > > > [214] nr_node_lock()
> > > > > > [253] nr_node->routes[2].neighbour = nr_neigh
> > > > > > [254] nr_neigh_hold(nr_neigh); [97] nr_add_node()
> > > > > > [XXX] nr_neigh_put()
> > > > > > ^^^^^^^^^^^^^^^^^^^^
> > > > > >
> > > > > > These charts are supposed to be chronological so [XXX] is wrong because the
> > > > > > use after free happens on line [248]. Do we really need three threads to
> > > > > > make this race work?
> > > > > The UAF problem occurs in Task2. Task1 sets the refcount of nr_neigh to 1,
> > > > > then Task0 adds it to routes[2]. Task2 releases routes[2].neighbour after
> > > > > executing [XXX]nr_neigh_put().
> > > > Execution Order:
> > > > 1 -> Task0
> > > > [113] nr_neigh_get_dev() // After execution, the refcount value is 3
> > > >
> > > > 2 -> Task1
> > > > [246] nr_neigh_put(nr_node->routes[2].neighbour); // After execution, the refcount value is 2
> > > > [248] nr_remove_neigh(nr_node->routes[2].neighbour) // After execution, the refcount value is 1
> > > >
> > > > 3 -> Task0
> > > > [253] nr_node->routes[2].neighbour = nr_neigh // nr_neigh's refcount value is 1 and add it to routes[2]
> > > >
> > > > 4 -> Task2
> > > > [XXX] nr_neigh_put(nr_node->routes[2].neighbour) // After execution, neighhour is freed
> > > > if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked) // Uaf occurs this line when accessing neighbour->count
> > >
> > > Let's step back a bit and look at the bigger picture design. (Which is
> > > completely undocumented so we're just guessing).
> > >
> > > When we put nr_neigh into nr_node->routes[] we bump the nr_neigh_hold()
> > > reference count and nr_neigh->count++, then when we remove it from
> > > ->routes[] we drop the reference and do nr_neigh->count--.
> > >
> > > If it's the last reference (and we are not holding ->locked) then we
> > > remove it from the &nr_neigh_list and drop the reference count again and
> > > free it. So we drop the reference count twice. This is a complicated
> > > design with three variables: nr_neigh_hold(), nr_neigh->count and
> > > ->locked. Why can it not just be one counter nr_neigh_hold(). So
> > > instead of setting locked = true we would just take an extra reference?
> > > The nr_neigh->count++ would be replaced with nr_neigh_hold() as well.
> > locked controls whether the neighbor quality can be automatically updated;
>
> I'm not sure your patch fixes the bug because we could still race against
> nr_del_node().
This is fine in this issue, but for rigor, I'll add locks to all related
ioctl and route frame operations to maintain synchronization.
I will send V3 patch to improve it.
>
> I'm not saying get rid of locked completely, I'm saying get rid of code like
> this:
> if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked)
> nr_remove_neigh(nr_node->routes[2].neighbour);
>
> Right now, locked serves as a special kind of reference count, because we
> don't drop the reference if it's true.
I don't think this is correct.
>
> > count controls the number of different routes a neighbor is linked to;
>
> Sure, that is interesting information for the user, so keep it around to
> print in the proc file, but don't use it as a reference count.
>
> > refcount is simply used to manage the neighbor lifecycle.
>
> The bug is caused because our reference counting is bad.
>
> So right now what happens is we allocate nr_neigh and we put it on the
> &nr_neigh_list. Then we lock it or we add it to ->routes[] and each of
> those has a different reference count. Then when we drop those references
> we do:
>
> if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked)
> nr_remove_neigh(nr_node->routes[2].neighbour);
>
> This removes it from the list, and hopefully this is the last reference
> and it frees it.
>
> It would be much simpler to say, we only use nr_neigh_hold()/put() for
> reference counting. When we set locked we do:
>
> nr_neigh_hold(nr_neigh);
> nr_neigh->locked = true;
>
> Incrementing the refcount means it can't be freed.
No, setting locked = 1 is only done in nr_add_neigh(), and nr_neigh_hold()
is not executed, and the refcount value is 1.
>
> Then when we remove nr_neigh from ->routes[] we wouldn't "remove it from
> the list", instead we would just drop a reference. When we dropped the
> last reference, nr_neigh_put() would remove it from the list.
>
> My proposal would be a behavior change because right now what happens is:
>
> 1: allocate nr_neigh
> 2: add it to ->routes[]
> 3: remove it from ->routes[]
> (freed automatically because we drop two references)
No, No, I know where your analysis went wrong, it is here.

The problem is not when allocating neigh and adding it to routes[2],
but when nr_add_node is executed twice later, one is Task0 as I mentioned
above, and the other is Task1.
After Task1 moves the neighbor out of routes, Task0 uses nr_neigh_get_dev()
to get the neighbor that Task1 moved out, and then adds it to its routes.
This is wrong. Task0 should not use nr_neigh_get_dev() to obtain the neighbor
before other tasks move it out. This will interfere with the reference count
of the neighbor, which is the root cause of the problem.

BR,
Lizhi