On Sat, Nov 30, 2019 at 10:10 AM James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > The two major core > changes are Al Viro's reworking of sg's handling of copy to/from user, > Ming Lei's removal of the host busy counter to avoid contention in the > multiqueue case and Damien Le Moal's fixing of residual tracking across > error handling. Math is hard. You say "The two major core changes are.." and then you list _three_ changes. Anyway, the sg copyin/out changes by Al conflicted fairly badly with Arnd's compat_ioctl changes. Al did c35a5cfb4150 ("scsi: sg: sg_read(): simplify reading ->pack_id of userland sg_io_hdr_t") which avoided doing a whole allocation of an 'sg_io_hdr_t' to just read the one field of it. But Arnd did 98aaaec4a150 ("compat_ioctl: reimplement SG_IO handling") which created a get_sg_io_hdr() helper that copied the 'sg_io_hdr_t' from user space the right way for both compat and native, which basically relied on the old approach. So I effectively reverted Al's patch in order to take Arnd's patch in the crazy sg legacy case that presumably nobody really cares about anyway, since everybody should use SG_IO rather than the sg_read() thing. But I know not everybody is. I added a comment in that place: /* * This is stupid. * * We're copying the whole sg_io_hdr_t from user * space just to get the 'pack_id' field. But the * field is at different offsets for the compat * case, so we'll use "get_sg_io_hdr()" to copy * the whole thing and convert it. * * We could do something like just calculating the * offset based of 'in_compat_syscall()', but the * 'compat_sg_io_hdr' definition is in the wrong * place for that. */ since it turns out that the one 'pack_id' field we want does have the same format in compat mode as in native mode ("int" and "compat_int_t" are the same), it's just at different offsets. But the definition of 'compat_sg_io_hdr' isn't available in that place. I'm leaving it to Al and Arnd to decide if they want to fix the stupidity. I tried to make the minimally invasive merge resolution. Al, Arnd? Comments? It looks like linux-next punted on this entirely, and took Al's simplified version that doesn't work with the compat case. Maybe I should have done the same - if you use read() on the /dev/sg* device, you deserve to get broken for the compat case. And it didn't historically work anyway. But it was kind of sad to see how Arnd fixed it, and then it got broken again. I really really wish we could get rid of sg_read/sg_write() entirely, and have SG_IO_SUBMIT and SG_IO_RECEIVE ioctl's that can handle the queued cases that apparently some people need. Because the read/write case really is disgusting. Linus