Re: [PATCH net] inetpeer: fix uninit-value in inet_getpeer

2018-04-09 Thread Eric Dumazet


On 04/09/2018 07:58 AM, David Miller wrote:
> From: Eric Dumazet 
> Date: Mon,  9 Apr 2018 06:43:27 -0700
> 
>> syzbot/KMSAN reported that p->dtime was read while it was
>> not yet initialized in :
>>
>>  delta = (__u32)jiffies - p->dtime;
>>  if (delta < ttl || !refcount_dec_if_one(&p->refcnt))
>>  gc_stack[i] = NULL;
>>
>> This is a false positive, because the inetpeer wont be erased
>> from rb-tree if the refcount_dec_if_one(&p->refcnt) does not
>> succeed. And this wont happen before first inet_putpeer() call
>> for this inetpeer has been done, and ->dtime field is written
>> exactly before the refcount_dec_and_test(&p->refcnt).
>>
>> The KMSAN report was :
>  ...
>> Signed-off-by: Eric Dumazet 
>> Reported-by: syzbot 
> 
> Applied, but it looks like we are just adding assignments simply
> to placate these reports when the tools and facilities cannot
> see through the logic properly.
> 

To be fair, this is because the check on ->dtime should be done a second time 
after
the refcount_dec_if_one(&p->refcnt)

It is a tiny race, and we do not really care given nature of inetpeer cache, 
best effort,
and DDOS candidate anyway.

If we purge one entry too soon, this is not a big deal.

I believe tool is fine.


Re: [PATCH net] inetpeer: fix uninit-value in inet_getpeer

2018-04-09 Thread David Miller
From: Eric Dumazet 
Date: Mon,  9 Apr 2018 06:43:27 -0700

> syzbot/KMSAN reported that p->dtime was read while it was
> not yet initialized in :
> 
>   delta = (__u32)jiffies - p->dtime;
>   if (delta < ttl || !refcount_dec_if_one(&p->refcnt))
>   gc_stack[i] = NULL;
> 
> This is a false positive, because the inetpeer wont be erased
> from rb-tree if the refcount_dec_if_one(&p->refcnt) does not
> succeed. And this wont happen before first inet_putpeer() call
> for this inetpeer has been done, and ->dtime field is written
> exactly before the refcount_dec_and_test(&p->refcnt).
> 
> The KMSAN report was :
 ...
> Signed-off-by: Eric Dumazet 
> Reported-by: syzbot 

Applied, but it looks like we are just adding assignments simply
to placate these reports when the tools and facilities cannot
see through the logic properly.