Re: [PATCH] ipv6/addrconf: fix timing bug in tempaddr regen

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2022-05-25 at 14:07 -0600, Sam Edwards wrote:
> Bah, I've had to resend this since it went out as HTML yesterday.
> Sorry about the double mailing everyone!
> 
> On Tue, May 24, 2022 at 3:24 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > I looks like with this change the tmp addresses will never hit the
> > DEPRECATED branch ?!?
> 
> The DEPRECATED branch becomes reachable again once this line is hit:
> ifp->regen_count++;
> ...because it causes this condition in the elseif to evaluate false:
> !ifp->regen_count

That condition looks problematic:


	unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
                                        ifp->idev->cnf.dad_transmits *
                                        max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;

	if (age >= ifp->prefered_lft - regen_advance) {

'age', 'ifp->prefered_lft' and 'regen_advance' are unsigned, and it
looks like 'regen_advance' is not constrained to be less then 'ifp-
>prefered_lft'. If that happens the condition will (allways) evaluate
to false, there will be no temporary address regenaration,
'regen_count' will be untouched and the temporary address will never
expire...

... unless I missed something relevant, which is totally possible ;)

Otherwise I think we need to explicitly handle the 'regen_advance >
ifp->prefered_lft' condition possibly with something alike:

	unsigned long regen_advance = ifp->idev->cnf.regen_max_retry * //...
	
	regen_adavance = min(regen_advance, ifp->prefered_lft);
	if (age >= ifp->prefered_lft - regen_advance) { //...

Thanks,

Paolo