| Message ID | 20230329184055.1307648-7-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | Turn single segment imports into ITER_UBUF | expand |
On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote: > > + struct iovec __ubuf_iovec; I think this is the third time I say this: this should be "const struct iovec". No other use is ever valid, and this cast: > +static inline const struct iovec *iter_iov(const struct iov_iter *iter) > +{ > + if (iter->iter_type == ITER_UBUF) > + return (const struct iovec *) &iter->__ubuf_iovec; should simply not exist. The first rule of cast club is that casting one pointer to another is generally a sign that something is wrong. Casting a pointer to an integer? That's a valid way to get at the bit representation for things like tagged pointers or for virtual address arithmetic etc (ok, and by "valid" I mean "valid in kernel contexts - not necessarily in all other contexts). Casting an integer to a pointer? Same thing - some things just need to do bit operations on what will become a pointer (allocators etc). But casting a pointer to another one - that should basically always raise eyebrows. You should try really hard to avoid it. Yes, we do it in the kernel, and yes, it *can* be valid. But most of the time it's really a bad sign. Linus
On 3/29/23 1:30 PM, Linus Torvalds wrote: > On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote: >> >> + struct iovec __ubuf_iovec; > > I think this is the third time I say this: this should be "const struct iovec". Doh sorry, not sure why I keep missing that... But yes, it should, I'll make the edit and actually amend it. > No other use is ever valid, and this cast: > >> +static inline const struct iovec *iter_iov(const struct iov_iter *iter) >> +{ >> + if (iter->iter_type == ITER_UBUF) >> + return (const struct iovec *) &iter->__ubuf_iovec; > > should simply not exist. Yep. Fixed both up.
On 3/29/23 1:38 PM, Jens Axboe wrote: > On 3/29/23 1:30 PM, Linus Torvalds wrote: >> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote: >>> >>> + struct iovec __ubuf_iovec; >> >> I think this is the third time I say this: this should be "const struct iovec". > > Doh sorry, not sure why I keep missing that... But yes, it should, I'll make > the edit and actually amend it. Now I recall why that ended up like that again, during the initial fiddling with this. If we leave it const, we get: CC arch/arm64/kernel/asm-offsets.s In file included from ./include/linux/socket.h:8, from ./include/linux/compat.h:15, from ./arch/arm64/include/asm/ftrace.h:52, from ./include/linux/ftrace.h:23, from arch/arm64/kernel/asm-offsets.c:12: ./include/linux/uio.h: In function ‘iov_iter_ubuf’: ./include/linux/uio.h:374:12: error: assignment of read-only location ‘*i’ 374 | *i = (struct iov_iter) { | ^ make[1]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1 make: *** [Makefile:1286: prepare0] Error 2 Let me take a closer look at that...
On 3/29/23 1:42 PM, Jens Axboe wrote: > On 3/29/23 1:38 PM, Jens Axboe wrote: >> On 3/29/23 1:30 PM, Linus Torvalds wrote: >>> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote: >>>> >>>> + struct iovec __ubuf_iovec; >>> >>> I think this is the third time I say this: this should be "const struct iovec". >> >> Doh sorry, not sure why I keep missing that... But yes, it should, I'll make >> the edit and actually amend it. > > Now I recall why that ended up like that again, during the initial fiddling > with this. If we leave it const, we get: > > CC arch/arm64/kernel/asm-offsets.s > In file included from ./include/linux/socket.h:8, > from ./include/linux/compat.h:15, > from ./arch/arm64/include/asm/ftrace.h:52, > from ./include/linux/ftrace.h:23, > from arch/arm64/kernel/asm-offsets.c:12: > ./include/linux/uio.h: In function ‘iov_iter_ubuf’: > ./include/linux/uio.h:374:12: error: assignment of read-only location ‘*i’ > 374 | *i = (struct iov_iter) { > | ^ > make[1]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1 > make: *** [Makefile:1286: prepare0] Error 2 > > Let me take a closer look at that... We can get rid of these if we convert the iov_iter initializers to just assign the members rather than the copy+zero fill. The automatic zero fill is nice though, in terms of sanity.
On Wed, Mar 29, 2023 at 12:49 PM Jens Axboe <axboe@kernel.dk> wrote: > > We can get rid of these if we convert the iov_iter initializers to > just assign the members rather than the copy+zero fill. The automatic > zero fill is nice though, in terms of sanity. The automatic zero fill is good, but I think it should be fixed by just not making that const struct iovec __ubuf_iovec; member be the first member of a union. The way union initializers work is that if they aren't named, they are for the first member. So I *think* the reason you get that warning is literally just because the __ubuf_iovec member is first in that union, and moving it down to below the other struct will just fix things. Linus
On 3/29/23 1:52 PM, Linus Torvalds wrote: > On Wed, Mar 29, 2023 at 12:49 PM Jens Axboe <axboe@kernel.dk> wrote: >> >> We can get rid of these if we convert the iov_iter initializers to >> just assign the members rather than the copy+zero fill. The automatic >> zero fill is nice though, in terms of sanity. > > The automatic zero fill is good, but I think it should be fixed by > just not making that > > const struct iovec __ubuf_iovec; > > member be the first member of a union. > > The way union initializers work is that if they aren't named, they are > for the first member. > > So I *think* the reason you get that warning is literally just because > the __ubuf_iovec member is first in that union, and moving it down to > below the other struct will just fix things. Nope, still fails with it moved below.
On Wed, Mar 29, 2023 at 12:56 PM Jens Axboe <axboe@kernel.dk> wrote: > > Nope, still fails with it moved below. Ouch. I guess the 'const' cast may be the only way then. It sounds like gcc may warn whenever it sees an assignment to any structure that has a const member, rather than when it sees an assignment to that particular member. Sad. Linus
On 3/29/23 1:59 PM, Linus Torvalds wrote: > On Wed, Mar 29, 2023 at 12:56 PM Jens Axboe <axboe@kernel.dk> wrote: >> >> Nope, still fails with it moved below. > > Ouch. I guess the 'const' cast may be the only way then. It sounds > like gcc may warn whenever it sees an assignment to any structure that > has a const member, rather than when it sees an assignment to that > particular member. > > Sad. Yeah, I tried a bunch of variants to trick it, including having it in a union with another non-const iovec first. But it cannot be tricked, so I think we're stuck with that. I'll add a comment.
diff --git a/include/linux/uio.h b/include/linux/uio.h index 5dbd2dcab35c..361688b86291 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -49,15 +49,30 @@ struct iov_iter { size_t iov_offset; int last_offset; }; - size_t count; + /* + * Hack alert: overlay ubuf_iovec with iovec + count, so + * that the members resolve correctly regardless of the type + * of iterator used. This means that you can use: + * + * &iter->ubuf or iter->iov + * + * interchangably for the user_backed cases, hence simplifying + * some of the cases that need to deal with both. + */ union { - /* use iter_iov() to get the current vec */ - const struct iovec *__iov; - const struct kvec *kvec; - const struct bio_vec *bvec; - struct xarray *xarray; - struct pipe_inode_info *pipe; - void __user *ubuf; + struct iovec __ubuf_iovec; + struct { + union { + /* use iter_iov() to get the current vec */ + const struct iovec *__iov; + const struct kvec *kvec; + const struct bio_vec *bvec; + struct xarray *xarray; + struct pipe_inode_info *pipe; + void __user *ubuf; + }; + size_t count; + }; }; union { unsigned long nr_segs; @@ -69,7 +84,13 @@ struct iov_iter { }; }; -#define iter_iov(iter) (iter)->__iov +static inline const struct iovec *iter_iov(const struct iov_iter *iter) +{ + if (iter->iter_type == ITER_UBUF) + return (const struct iovec *) &iter->__ubuf_iovec; + return iter->__iov; +} + #define iter_iov_addr(iter) (iter_iov(iter)->iov_base + (iter)->iov_offset) #define iter_iov_len(iter) (iter_iov(iter)->iov_len - (iter)->iov_offset)
Add an internal struct iovec that we can return as a pointer, with the fields of the iovec overlapping with the ITER_UBUF ubuf and length fields. Then we can have iter_iov() check for the appropriate type, and return &iter->__ubuf_iovec for ITER_UBUF and iter->__iov for ITER_IOVEC and things will magically work out for a single segment request regardless of either type. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- include/linux/uio.h | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-)