On Wed 16-03-22 22:14:50, Tetsuo Handa wrote:
> On 2022/03/16 20:22, Jan Kara wrote:
> >> @@ -1244,7 +1244,7 @@ static int loop_clr_fd(struct loop_device *lo)
> >> * <dev>/do something like mkfs/losetup -d <dev> causing the losetup -d
> >> * command to fail with EBUSY.
> >> */
> >> - if (atomic_read(&lo->lo_refcnt) > 1) {
> >> + if (lo->lo_disk->part0->bd_openers > 1) {
> >
> > But bd_openers can be read safely only under disk->open_mutex. So for this
> > to be safe against compiler playing nasty tricks with optimizations, we
> > need to either make bd_openers atomic_t or use READ_ONCE / WRITE_ONCE when
> > accessing it.
>
> Use of READ_ONCE() / WRITE_ONCE() are not for avoiding races but for making
> sure that memory access happens only once. It is data_race() which is needed
> for tolerating and annotating races. For example, data_race(lo->lo_state) is
> needed when accessing lo->lo_state without lo->lo_mutex held.
Well, but another effect of READ_ONCE() / WRITE_ONCE() is that it
effectively forces the compiler to not store any intermediate value in
bd_openers. If you have code like bdev->bd_openers++, and bd_openers has
value say 1, the compiler is fully within its rights if unlocked reader
sees values, 1, 0, 3, 2. It would have to be a vicious compiler but the C
standard allows that and some of the optimizations compilers end up doing
result in code which is not far from this (read more about KCSAN and the
motivation behind it for details). So data_race() annotation is *not*
enough for unlocked bd_openers usage.
> Use of atomic_t for lo->lo_disk->part0->bd_openers does not help, for
> currently lo->lo_mutex is held in order to avoid races. That is, it is
> disk->open_mutex which loop_clr_fd() needs to hold when accessing
> lo->lo_disk->part0->bd_openers.
It does help because with atomic_t, seeing any intermediate values is not
possible even for unlocked readers.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
