| Message ID | ZdjTMZRwZ_9GjCmc@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | [git,pull] device mapper fixes for 6.8-rc6 | expand |
On Fri, 23 Feb 2024 at 09:17, Mike Snitzer <snitzer@kernel.org> wrote: > > - Fix DM crypt and verity targets to align their respective bvec_iter > struct members to avoid the need for byte level access (due to > __packed attribute) that is costly on some arches (like RISC). Ugh. This is due to commit 19416123ab3e ("block: define 'struct bvec_iter' as packed"), and the point of *that* commit was that it doesn't hurt to mark it packed. That was clearly not true. And honestly, "__packed" really is wrong here. Nobody ever wanted it to be completely unaligned. I think we might be better off marking it as being 4-byte aligned. That would mean that instead of __packed, it is done as __packed __aligned(4) because "__aligned" on its own only increases alignment (so without the __packed it would stay 8-byte aligned). Then the only part of that structure that might be unaligned is "sector_t", and that would only matter on 64-bit architectures. And very few architectures are both 64-bit _and_ so broken as to not do unaligned loads well. And even if such broken architectures exist, at least they can do the 8-byte load as two 4-byte ones rather than doing it as byte loads.. Anyway, I've pulled this, but I really think this should have been fixed in bvec.h instead. Jens/Christoph/whoever feels they own bvec.h? Linus
On Fri, Feb 23, 2024 at 09:42:21AM -0800, Linus Torvalds wrote: > And honestly, "__packed" really is wrong here. Nobody ever wanted it > to be completely unaligned. I'll let Ming speak, but I think the idea was to remove the padding at the end of the structure when embedded into the bio. > > I think we might be better off marking it as being 4-byte aligned. > That would mean that instead of __packed, it is done as > > __packed __aligned(4) > > > because "__aligned" on its own only increases alignment (so without > the __packed it would stay 8-byte aligned). > > Then the only part of that structure that might be unaligned is > "sector_t", and that would only matter on 64-bit architectures. Does __aligned also work on struct members? If so we could add a __aligned(8) to bi_sector an get exactly what we want..
On Fri, 23 Feb 2024 at 09:46, Christoph Hellwig <hch@lst.de> wrote: > > I'll let Ming speak, but I think the idea was to remove the padding > at the end of the structure when embedded into the bio. It's not horribly obvious if the beginning is aligned there either. > Does __aligned also work on struct members? If so we could add a > __aligned(8) to bi_sector an get exactly what we want.. Hmm. I'm not sure that works. I think sizeof may always end up being aligned to alignof (because otherwise arrays cannot work) And looking at struct bio_integrity_payload { there's odd padding both before _and_ after the struct bvec_iter due to having three 16-bit fields in between. So right now I think that packing ends up actually horrid. I don't see any *reason* for that odd setup, but right now it might have actually end up being 2-byte aligned with a two-byte padding hole at the end. Linus
The pull request you sent on Fri, 23 Feb 2024 12:17:37 -0500:
> git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.8/dm-fixes-2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e7768e65cd777182553b98f5b4eaffb624974976
Thank you!
Hi Linus, The following changes since commit 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478: Linux 6.8-rc3 (2024-02-04 12:20:36 +0000) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.8/dm-fixes-2 for you to fetch changes up to 0e0c50e85a364bb7f1c52c84affe7f9e88c57da7: dm-crypt, dm-integrity, dm-verity: bump target version (2024-02-20 13:35:47 -0500) Please pull, thanks. Mike ---------------------------------------------------------------- - Stable fixes for 3 DM targets (integrity, verity and crypt) to address systemic failure that can occur if user provided pages map to the same block. - Fix DM crypt to not allow modifying data that being encrypted for authenticated encryption. - Fix DM crypt and verity targets to align their respective bvec_iter struct members to avoid the need for byte level access (due to __packed attribute) that is costly on some arches (like RISC). -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEJfWUX4UqZ4x1O2wixSPxCi2dA1oFAmXY0iwACgkQxSPxCi2d A1oG3Qf/WE0T5qyBnDZ7irhvJmSLVx4oAwzB0PmMtELZ3Tkyn7BBAxq1Q2I2UT3x r90d1uy/pz6Y+kZkAPZjYuYLctukEa1swpfFe0Sn01dBrbgGU/p2vi3fkF+ZK6/t n5EN8S5dkf6rIDmp8R56iP8mP4OEultYjLugxc6ROohFgHZicoqv+Pye9kHp0Y19 HSW2eueag/s2nMa9HKjIEd3+NBgmGb0qMMf3M6CXpRLNi/f/cyHbPzq83+eW3gcg jl480w5YHk2nOUSqrO8UfIaP4BpD3SEXQxVqIzdkVX4cEBO4yRcBNrQpsT89GsXj sg5zinkq3g7SThEpQWdpkeZMR/6q/A== =n0nQ -----END PGP SIGNATURE----- ---------------------------------------------------------------- Mike Snitzer (1): dm-crypt, dm-integrity, dm-verity: bump target version Mikulas Patocka (5): dm-integrity: recheck the integrity tag after a failure dm-verity: recheck the hash after a failure dm-crypt: don't modify the data when using authenticated encryption dm-crypt: recheck the integrity tag after a failure dm-verity, dm-crypt: align "struct bvec_iter" correctly drivers/md/dm-crypt.c | 101 ++++++++++++++++++++++++++++++++++-------- drivers/md/dm-integrity.c | 95 ++++++++++++++++++++++++++++++++++----- drivers/md/dm-verity-target.c | 88 +++++++++++++++++++++++++++++++++--- drivers/md/dm-verity.h | 10 ++++- 4 files changed, 256 insertions(+), 38 deletions(-)