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

Dan Carpenter
2025-10-20T12:57:39Z

On Mon, Oct 20, 2025 at 03:25:40PM +0300, Dan Carpenter wrote:
> netrom/nr_route.c
> 214 nr_node_lock(nr_node);

I guess nr_node is different for each thread?

> 215
> 216 if (quality != 0)
> 217 strscpy(nr_node->mnemonic, mnemonic);
> 218
> 219 for (found = 0, i = 0; i < nr_node->count; i++) {
> 220 if (nr_node->routes[i].neighbour == nr_neigh) {
> 221 nr_node->routes[i].quality = quality;
> 222 nr_node->routes[i].obs_count = obs_count;
> 223 found = 1;
> 224 break;
> 225 }
> 226 }
> 227
> 228 if (!found) {
> 229 /* We have space at the bottom, slot it in */
> 230 if (nr_node->count < 3) {
> 231 nr_node->routes[2] = nr_node->routes[1];
> 232 nr_node->routes[1] = nr_node->routes[0];
> 233
> 234 nr_node->routes[0].quality = quality;
> 235 nr_node->routes[0].obs_count = obs_count;
> 236 nr_node->routes[0].neighbour = nr_neigh;
> 237
> 238 nr_node->which++;
> 239 nr_node->count++;
> 240 nr_neigh_hold(nr_neigh);
> 241 nr_neigh->count++;
> 242 } else {
> 243 /* It must be better than the worst */
> 244 if (quality > nr_node->routes[2].quality) {
> 245 nr_node->routes[2].neighbour->count--;
> 246 nr_neigh_put(nr_node->routes[2].neighbour);
> 247
> 248 if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked)
> 249 nr_remove_neigh(nr_node->routes[2].neighbour);

Does neighbour->count means how many nr_node pointers have it in ->routes[]?
I wish this code had comments.
KTDOO: add comments explaining all the counters in netrom/nr_route.c

> 250
> 251 nr_node->routes[2].quality = quality;
> 252 nr_node->routes[2].obs_count = obs_count;
> 253 nr_node->routes[2].neighbour = nr_neigh;
> 254
> 255 nr_neigh_hold(nr_neigh);
> 256 nr_neigh->count++;

Could we just add some locking to this if statement only? I had thought
that nr_node_lock() serialized it but now I think maybe not? Or maybe we
could increment the counters before assigning it?

nr_neigh->count++;
nr_neigh_hold(nr_neigh);
nr_node->routes[2].neighbour = nr_neigh;

I'm not an expert in concerency. Does calling nr_neigh_hold() mean th
that the nr_neigh->count++ will happen before the assignment?

> 257 }
> 258 }
> 259 }

regards,
dan carpenter