linux-kernel - Re: [PATCH] btrfs: fix a potential racy

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] [day] [month] [year] [list]

Message-ID: <20200422214526.GT18421@twin.jikos.cz>
Date:   Wed, 22 Apr 2020 23:45:26 +0200
From:   David Sterba <dsterba@...e.cz>
To:     wu000273@....edu
Cc:     clm@...com, josef@...icpanda.com, dsterba@...e.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        kjlu@....edu
Subject: Re: [PATCH] btrfs: fix a potential racy

On Sat, Apr 18, 2020 at 08:59:07PM -0500, wu000273@....edu wrote:
> From: Qiushi Wu <wu000273@....edu>
> 
> In function reada_find_extent and reada_extent_put, kref_get(&zone->refcnt)
> are not called in a lock context. Potential racy may happen. It's possible
> that thread1 decreases the kref to 0, and thread2 increases the kref to 1,
> then thread1 releases the pointer.

There are several things I don't see or understand why they woudl be a
problem.

kref_get does not need to be take under locks in case it's not the first
reference or if it's clear that eg. the caller has taken a reference and
it'll never go to 0.

The references are supposed to be lightweight so taking the locks per
kref_get does not follow a good pattern.

The kref type is a wrapper around refcount_t, that detects if there's an
increment from 0->1, similar to what you suggest that could happen. It
might be tricky to actually hit that case so I'm not saying that it's
all ok, just that we haven't seen that so far.

Your description of the race needs to be expanded as it's too terse,
where exactly the increments could happen, how would be the calls in the
two cpus interleaved, like

cpu1				cpu2

				reada_find_extent()
				   kref_get
				   ...
reada_find_extent()
   				   kref_put (last put, refs == 0)
   kref_get (inc from zero)
   ...

Then it's easer to reason about the actual races and the context where
it could happen. Eg. btrfs_reada_add adds one reference from the
beginning, so the final put does not happen in the middle of
reada_find_extent unless something would be seriously broken and that
would manifest very clearly.