| Message ID | b8f8fb6d1d10386c74f2d8826b737a74c60b76b2.1724743492.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Fixups for random vDSO | expand |
On Tue, Aug 27, 2024 at 09:31:48AM +0200, Christophe Leroy wrote: > - ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len); > + const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT; > + const unsigned long page_mask = ~(page_size - 1); > + ssize_t ret = min_t(size_t, INT_MAX & page_mask /* = MAX_RW_COUNT */, len); I'm really not a fan of making the code less idiomatic... > An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO > when they do not exist already, but this can be misleading. Why would what tglx and I suggested be misleading? That seems pretty normal... Are you worried they might mismatch somehow? (If so, why?) Jason
Le 27/08/2024 à 09:49, Jason A. Donenfeld a écrit : > On Tue, Aug 27, 2024 at 09:31:48AM +0200, Christophe Leroy wrote: >> - ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len); >> + const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT; >> + const unsigned long page_mask = ~(page_size - 1); >> + ssize_t ret = min_t(size_t, INT_MAX & page_mask /* = MAX_RW_COUNT */, len); > > I'm really not a fan of making the code less idiomatic... Ok, I have another idea, let's give it a try. > >> An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO >> when they do not exist already, but this can be misleading. > > Why would what tglx and I suggested be misleading? That seems pretty > normal... Are you worried they might mismatch somehow? (If so, why?) All architectures have their own definition, they are all based on CONFIG_PAGE_SHIFT and should give the same value but with some subtleties. The best would be to have an asm-generic definition of PAGE_SIZE and PAGE_MASK that all architectures use, but that's another level of work. tglx or yourself suggested to put in a one of the vdso headers instead of directly in getrandom.c. This is too fragile because PAGE_SIZE might be absent in that header but arrive in getrandom.c through another header. Christophe
On Tue, Aug 27, 2024 at 10:16:18AM +0200, Christophe Leroy wrote: > tglx or yourself suggested to put in a one of the vdso headers instead > of directly in getrandom.c. This is too fragile because PAGE_SIZE might > be absent in that header but arrive in getrandom.c through another header. Oh jeeze, yea. FYI, _PAGE_SIZE is defined on s390, so that might not be such a good idea (from the previous version). > The best would be to have an asm-generic definition of > PAGE_SIZE and PAGE_MASK that all architectures use, but that's another > level of work. Yea that seems far too large of an operation to do here. > > I'm really not a fan of making the code less idiomatic... > > Ok, I have another idea, let's give it a try. What's the other idea?
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c index f1643656d0b0..5874e3072bfe 100644 --- a/lib/vdso/getrandom.c +++ b/lib/vdso/getrandom.c @@ -65,7 +65,9 @@ static __always_inline ssize_t __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len) { - ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len); + const unsigned long page_size = 1UL << CONFIG_PAGE_SHIFT; + const unsigned long page_mask = ~(page_size - 1); + ssize_t ret = min_t(size_t, INT_MAX & page_mask /* = MAX_RW_COUNT */, len); struct vgetrandom_state *state = opaque_state; size_t batch_len, nblocks, orig_len = len; bool in_use, have_retried = false; @@ -84,7 +86,7 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_ } /* The state must not straddle a page, since pages can be zeroed at any time. */ - if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE)) + if (unlikely(((unsigned long)opaque_state & ~page_mask) + sizeof(*state) > page_size)) return -EFAULT; /* Handle unexpected flags by falling back to the kernel. */
Using PAGE_SIZE and PAGE_MASK in VDSO requires inclusion of page.h and it creates several problems, see commit 8b3843ae3634 ("vdso/datapage: Quick fix - use asm/page-def.h for ARM64") and commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in vdso/datapage.h"). An easy solution would be to define PAGE_SIZE and PAGE_MASK in vDSO when they do not exist already, but this can be misleading. So follow the same approach as commit cffaefd15a8f ("vdso: Use CONFIG_PAGE_SHIFT in vdso/datapage.h") and exclusively use CONFIG_PAGE_SHIFT. To avoid too much ugliness, define local consts that constains the calculated page size and page mask. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- v3: Use local consts instead of _PAGE_SIZE and _PAGE_MASK macros that are already defined by some architectures. --- lib/vdso/getrandom.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)