netdev - Re: [PATCH net] inet: frags: fix a race between inet_evict_bucket and inet_frag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141028100315.GA14863@breakpoint.cc>
Date:	Tue, 28 Oct 2014 11:03:15 +0100
From:	Florian Westphal <fw@...len.de>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	netdev@...r.kernel.org, Florian Westphal <fw@...len.de>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Patrick McLean <chutzpah@...too.org>
Subject: Re: [PATCH net] inet: frags: fix a race between inet_evict_bucket
 and inet_frag_kill

Nikolay Aleksandrov <nikolay@...hat.com> wrote:
> When the evictor is running it adds some chosen frags to a local list to
> be evicted once the chain lock has been released but at the same time
> the *frag_queue can be running for some of the same queues and it
> may call inet_frag_kill which will wait on the chain lock and
> will then delete the queue from the wrong list since it was added in the
> eviction one.

I had to read that twice...

cpu1						cpu2
inet_evict_bucket				inet_frag_kill
  chain_lock()                                    chain_lock() ..
  for_each_frag_queue                               spin
    set fragqueue INET_FRAG_EVICTED flag [A]        .
    hlist_del()                                     spin
    hlist_add (to private list)                     .
                                                    spin
  chain_unlock                                      .
						    chain_lock returns
  for_each_frag_queue_on_private_list		    hlist_del() [B]
     frag_expire(fq) // destroy/free queue

[B] we may delete entry on the evictors private list.

since [A] is only set with chainlock held, other cpus
killing an entry can use INET_FRAG_EVICTED to test if the
entry is about to be removed by the evictor.

> The fix is simple - check if the queue has the evict flag
> set under the chain lock before deleting it, this is safe because the
> evict flag is set only under that lock and having the flag set also means
> that the queue has been detached from the chain list, so no need to delete
> it again.

Right, thanks everyone.
> ---
> A few more eyes to confirm all of this would be much appreciated.

Looks correct,
Reviewed-by: Florian Westphal <fw@...len.de>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html