| Message ID | 20230328173613.555192-5-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | Turn single segment imports into ITER_UBUF | expand |
On Tue, Mar 28, 2023 at 10:36 AM Jens Axboe <axboe@kernel.dk> wrote: > > @@ -3516,23 +3516,28 @@ static void __user **snd_map_bufs(struct snd_pcm_runtime *runtime, > struct iov_iter *iter, > snd_pcm_uframes_t *frames, int max_segs) > { > + int nr_segs = iovec_nr_user_vecs(iter); This has a WARN_ON_ONCE() for !user_backed, but then.. > void __user **bufs; > + struct iovec iov; > unsigned long i; > > if (!iter->user_backed) > return ERR_PTR(-EFAULT); here the code tries to deal with it. So I think the two should probably be switched around. Linus
On 3/28/23 11:50 AM, Linus Torvalds wrote: > On Tue, Mar 28, 2023 at 10:36 AM Jens Axboe <axboe@kernel.dk> wrote: >> >> @@ -3516,23 +3516,28 @@ static void __user **snd_map_bufs(struct snd_pcm_runtime *runtime, >> struct iov_iter *iter, >> snd_pcm_uframes_t *frames, int max_segs) >> { >> + int nr_segs = iovec_nr_user_vecs(iter); > > This has a WARN_ON_ONCE() for !user_backed, but then.. > >> void __user **bufs; >> + struct iovec iov; >> unsigned long i; >> >> if (!iter->user_backed) >> return ERR_PTR(-EFAULT); > > here the code tries to deal with it. > > So I think the two should probably be switched around. True, it was actually like that before I refactored it to include that common helper. I'll swap them around, thanks.
On Tue, Mar 28, 2023 at 11:52:10AM -0600, Jens Axboe wrote: > On 3/28/23 11:50 AM, Linus Torvalds wrote: > > On Tue, Mar 28, 2023 at 10:36 AM Jens Axboe <axboe@kernel.dk> wrote: > >> > >> @@ -3516,23 +3516,28 @@ static void __user **snd_map_bufs(struct snd_pcm_runtime *runtime, > >> struct iov_iter *iter, > >> snd_pcm_uframes_t *frames, int max_segs) > >> { > >> + int nr_segs = iovec_nr_user_vecs(iter); > > > > This has a WARN_ON_ONCE() for !user_backed, but then.. > > > >> void __user **bufs; > >> + struct iovec iov; > >> unsigned long i; > >> > >> if (!iter->user_backed) > >> return ERR_PTR(-EFAULT); > > > > here the code tries to deal with it. > > > > So I think the two should probably be switched around. > > True, it was actually like that before I refactored it to include > that common helper. I'll swap them around, thanks. Umm... That looks really weird - if nothing else, it seems that this thing quietly ignores the ->iov_len on all but the first iovec. Might make sense to ask ALSA folks what the hell is going on there; it's readv()/writev() on pcm device, and it looks like userland ABI is really perverted here... ;-/
On 3/28/23 12:52 PM, Al Viro wrote: > On Tue, Mar 28, 2023 at 11:52:10AM -0600, Jens Axboe wrote: >> On 3/28/23 11:50 AM, Linus Torvalds wrote: >>> On Tue, Mar 28, 2023 at 10:36 AM Jens Axboe <axboe@kernel.dk> wrote: >>>> >>>> @@ -3516,23 +3516,28 @@ static void __user **snd_map_bufs(struct snd_pcm_runtime *runtime, >>>> struct iov_iter *iter, >>>> snd_pcm_uframes_t *frames, int max_segs) >>>> { >>>> + int nr_segs = iovec_nr_user_vecs(iter); >>> >>> This has a WARN_ON_ONCE() for !user_backed, but then.. >>> >>>> void __user **bufs; >>>> + struct iovec iov; >>>> unsigned long i; >>>> >>>> if (!iter->user_backed) >>>> return ERR_PTR(-EFAULT); >>> >>> here the code tries to deal with it. >>> >>> So I think the two should probably be switched around. >> >> True, it was actually like that before I refactored it to include >> that common helper. I'll swap them around, thanks. > > Umm... That looks really weird - if nothing else, it seems that this > thing quietly ignores the ->iov_len on all but the first iovec. I agree, but this is how it currently works... > Might make sense to ask ALSA folks what the hell is going on there; > it's readv()/writev() on pcm device, and it looks like userland ABI > is really perverted here... ;-/ I have sent them email separately to confirm that the only cases that makes sense here is nr_segs >= 2. But the ABI is what it is, however horrible it may be :/
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 925d96343148..05913d68923a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3516,23 +3516,28 @@ static void __user **snd_map_bufs(struct snd_pcm_runtime *runtime, struct iov_iter *iter, snd_pcm_uframes_t *frames, int max_segs) { + int nr_segs = iovec_nr_user_vecs(iter); void __user **bufs; + struct iovec iov; unsigned long i; if (!iter->user_backed) return ERR_PTR(-EFAULT); - if (!iter->nr_segs) + if (!nr_segs) return ERR_PTR(-EINVAL); - if (iter->nr_segs > max_segs || iter->nr_segs != runtime->channels) + if (nr_segs > max_segs || nr_segs != runtime->channels) return ERR_PTR(-EINVAL); - if (!frame_aligned(runtime, iter->iov->iov_len)) + iov = iov_iter_iovec(iter); + if (!frame_aligned(runtime, iov.iov_len)) return ERR_PTR(-EINVAL); - bufs = kmalloc_array(iter->nr_segs, sizeof(void *), GFP_KERNEL); + bufs = kmalloc_array(nr_segs, sizeof(void *), GFP_KERNEL); if (bufs == NULL) return ERR_PTR(-ENOMEM); - for (i = 0; i < iter->nr_segs; ++i) + bufs[0] = iov.iov_base; + /* we know it's an ITER_IOVEC is nr_segs > 1 */ + for (i = 1; i < nr_segs; ++i) bufs[i] = iter->iov[i].iov_base; - *frames = bytes_to_samples(runtime, iter->iov->iov_len); + *frames = bytes_to_samples(runtime, iov.iov_len); return bufs; }
This probably doesn't make any sense, as it's reliant on passing in different things in multiple segments. Most likely we can just make this go away as it's already checking for ITER_IOVEC upon entry, and it looks like nr_segments == 2 is the smallest legal value. IOW, any attempt to readv/writev with 1 segment would fail with -EINVAL already. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- sound/core/pcm_native.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)