On Sat, Jul 02, 2022 at 06:35:58AM +0100, Al Viro wrote: > On Fri, Jul 01, 2022 at 01:56:53PM -0600, Keith Busch wrote: > > > Validating user requests gets really messy if we allow arbitrary segment > > lengths. This particular patch just enables arbitrary address alignment, but > > segment size is still required to be a block size. You found the commit that > > enforces that earlier, "iov: introduce iov_iter_aligned", two commits prior. > > BTW, where do you check it for this caller? > fs/zonefs/super.c:786: ret = bio_iov_iter_get_pages(bio, from); > Incidentally, we have an incorrect use of iov_iter_truncate() in that one (compare > with iomap case, where we reexpand it afterwards)... > > I still don't get the logics of those round-downs. You've *already* verified > that each segment is a multiple of logical block size. And you are stuffing > as much as you can into bio, covering the data for as many segments as you > can. Sure, you might end up e.g. running into an unmapped page at wrong > offset (since your requirements for initial offsets might be milder than > logical block size). Or you might run out of pages bio would take. Either > might put the end of bio at the wrong offset. It is strange this function allows the possibility that bio_iov_add_page() can fail. There's no reason to grab more pages that exceed the bio_full() condition (ignoring the special ZONE_APPEND case for the moment). I think it will make more sense if we clean that part up first so the size for all successfully gotten pages can skip subsequent bio add page checks, and make the error handling unnecessary. > So why not trim it down *after* you are done adding pages into it? And do it > once, outside of the loop. IDGI... Validation is already done; I'm not > suggesting to allow weird segment lengths or to change behaviour of your > iov_iter_is_aligned() in any other way. I may have misunderstood your previous suggestion, but I still think this is the right way to go. The ALIGN_DOWN in its current location ensures the size we're appending to the bio is acceptable before we even start. It's easier to prevent adding pages to a bio IO than to back them out later. The latter would need something like a special cased version of bio_truncate(). Anyway, I have some changes testing right now that I think will fix up the issues you've raised, and make the rest a bit more clear. I'll send them for consideration this weekend if all is succesful. > Put it another way, is there any possibility for __bio_iov_iter_get_pages() to > do a non-trivial round-down on anything other than the last iteration of that > loop?
