nd6: Avoid using an uninitialized sockaddr in nd6_prefix_offlink()

[markj]

Summary

Commit 81728a538 ("Split rtinit() into multiple functions.") removed
the initialization of sa6, but not one of its uses. This meant that we
were passing an uninitialized sockaddr as the address to
lltable_prefix_free(). Remove the variable outright to fix the problem.
The caller is expected to hold a reference on pr.

Reported by: KMSAN

[donner]

The free() is called outside of the interesting function an may be called on an still active component.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

sys/netinet6/nd6_rtr.c

2189

If an error occured, a_failure is 1 and will never be reset.

[markj]

In D30166#677401, @donner wrote:

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

I can do that in a follow-up change. Here I would like to provide a minimal patch for the regression to simplify backporting and reduce the risk of another regression. In general I agree this code needs some general cleanup.

sys/netinet6/nd6_rtr.c

2189

Are you saying that we should not call lltable_prefix_free() in that case?

[donner]

In D30166#678451, @markj wrote:

In D30166#677401, @donner wrote:

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

My fault. I lost some characters while typing.

The code structure is:

allocate(prefix);
fail = 1;
if(good)
    forall()
       eventually_reset_fail;
else
    do_not_touch_fail;
if(fail)
    free(prefix);

There is more than one way through the code, where prefix is consumed or not.
My question is if all cases are fulfilled.

I'd suggest to handle the error != 0 case first an return in this branch early.
So the main search loop shifts to the main row and the code is simpler.

I can do that in a follow-up change. Here I would like to provide a minimal patch for the regression to simplify backporting and reduce the risk of another regression. In general I agree this code needs some general cleanup.

Okay. Sounds good.

[markj]

In D30166#678454, @donner wrote:

In D30166#678451, @markj wrote:

In D30166#677401, @donner wrote:

The free() is called outside of the interesting function an may be called on an still active component.

Sorry, I'm having trouble understanding what you're suggesting here.

My fault. I lost some characters while typing.

The code structure is:

allocate(prefix);
fail = 1;
if(good)
    forall()
       eventually_reset_fail;
else
    do_not_touch_fail;
if(fail)
    free(prefix);

There is more than one way through the code, where prefix is consumed or not.
My question is if all cases are fulfilled.

So I think the current behaviour is more or less intentional. Even if the route deletion fails, we want to purge the LLE, as the prefix isn't supposed to be used anymore. IPv4 behaves the same way.