linux-kernel - Re: [PATCH 0/4] Fix a crash when block device is read and block size is changed at the same time

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: <Pine.LNX.4.64.1209251850380.21075@file.rdu.redhat.com>
Date:	Tue, 25 Sep 2012 18:58:24 -0400 (EDT)
From:	Mikulas Patocka <mpatocka@...hat.com>
To:	Jeff Moyer <jmoyer@...hat.com>
cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Jens Axboe <axboe@...nel.dk>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Jan Kara <jack@...e.cz>, dm-devel@...hat.com,
	linux-kernel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	kosaki.motohiro@...fujitsu.com, linux-fsdevel@...r.kernel.org,
	lwoodman@...hat.com, "Alasdair G. Kergon" <agk@...hat.com>
Subject: Re: [PATCH 0/4] Fix a crash when block device is read and block size
 is changed at the same time



On Tue, 25 Sep 2012, Jeff Moyer wrote:

> Jeff Moyer <jmoyer@...hat.com> writes:
> 
> > Mikulas Patocka <mpatocka@...hat.com> writes:
> >
> >> Hi Jeff
> >>
> >> Thanks for testing.
> >>
> >> It would be interesting ... what happens if you take the patch 3, leave 
> >> "struct percpu_rw_semaphore bd_block_size_semaphore" in "struct 
> >> block_device", but remove any use of the semaphore from fs/block_dev.c? - 
> >> will the performance be like unpatched kernel or like patch 3? It could be 
> >> that the change in the alignment affects performance on your CPU too, just 
> >> differently than on my CPU.
> >
> > It turns out to be exactly the same performance as with the 3rd patch
> > applied, so I guess it does have something to do with cache alignment.
> > Here is the patch (against vanilla) I ended up testing.  Let me know if
> > I've botched it somehow.
> >
> > So, I next up I'll play similar tricks to what you did (padding struct
> > block_device in all kernels) to eliminate the differences due to
> > structure alignment and provide a clear picture of what the locking
> > effects are.
> 
> After trying again with the same padding you used in the struct
> bdev_inode, I see no performance differences between any of the
> patches.  I tried bumping up the number of threads to saturate the
> number of cpus on a single NUMA node on my hardware, but that resulted
> in lower IOPS to the device, and hence consumption of less CPU time.
> So, I believe my results to be inconclusive.

For me, the fourth patch with RCU-based locks performed better, so I am 
submitting that.

> After talking with Vivek about the problem, he had mentioned that it
> might be worth investigating whether bd_block_size could be protected
> using SRCU.  I looked into it, and the one thing I couldn't reconcile is
> updating both the bd_block_size and the inode->i_blkbits at the same
> time.  It would involve (afaiui) adding fields to both the inode and the
> block_device data structures and using rcu_assign_pointer  and
> rcu_dereference to modify and access the fields, and both fields would
> need to protected by the same struct srcu_struct.  I'm not sure whether
> that's a desirable approach.  When I started to implement it, it got
> ugly pretty quickly.  What do others think?

Using RCU doesn't seem sensible to me (except for lock implementation, as 
it is in patch 4). The major problem is that the block layer reads 
blocksize multiple times and when different values are read, a crash may 
happen - RCU doesn't protect you against that - if you read a variable 
multiple times in a RCU-protected section, you can still get different 
results.

If we wanted to use RCU, we would have to read blocksize just once and 
pass the value between all functions involved - that would result in a 
massive code change.

> For now, my preference is to get the full patch set in.  I will continue
> to investigate the performance impact of the data structure size changes
> that I've been seeing.

Yes, we should get the patches to the kernel.

Mikulas

> So, for the four patches:
> 
> Acked-by: Jeff Moyer <jmoyer@...hat.com>
> 
> Jens, can you have a look at the patch set?  We are seeing problem
> reports of this in the wild[1][2].
> 
> Cheers,
> Jeff
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=824107
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=812129
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/