Re: [bug report] dead loop in generic_perform_write() //Re: [PATCH v7 07/12] iov_iter: Convert iterate*() to inline funcs

[Tong Tiangen]

在 2024/2/29 6:57, Linus Torvalds 写道:
> On Wed, 28 Feb 2024 at 13:21, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > Hmm. If the copy doesn't succeed and make any progress at all, then
> > the code in generic_perform_write() after the "goto again"
> >
> >                  //[4]
> >                  if (unlikely(fault_in_iov_iter_readable(i, bytes) ==
> >                                bytes)) {
> >
> > should break out of the loop.
>
> Ahh. I see the problem. Or at least part of it.
>
> The iter is an ITER_BVEC.
>
> And fault_in_iov_iter_readable() "knows" that an ITER_BVEC cannot
> fail. Because obviously it's a kernel address, so no user page fault.
>
> But for the machine check case, ITER_BVEC very much can fail.
>
> This should never have worked in the first place.
>
> What a crock.
>
> Do we need to make iterate_bvec() always succeed fully, and make
> copy_mc_to_kernel() zero out the end?
>
>                     Linus
> .

Hi Linus:

See the logic before this patch, always success (((void)(K),0)) is
returned for three types: ITER_BVEC, ITER_KVEC and ITER_XARRAY.

-------------------------------------------------------------------
  -#define __iterate_and_advance(i, n, base, len, off, I, K) {	\
  -	if (unlikely(i->count < n))				\
  -		n = i->count;					\
  -	if (likely(n)) {					\
  -		if (likely(iter_is_ubuf(i))) {			\
  			[...]					\
  -			iterate_buf(i, n, base, len, off,	\
  -						i->ubuf, (I)) 	\
  -		} else if (likely(iter_is_iovec(i))) {		\
			[...]					\
  -			iterate_iovec(i, n, base, len, off,	\
  -						iov, (I))	\
  -			i->nr_segs -= iov - iter_iov(i);	\
  -			i->__iov = iov;				\
  -		} else if (iov_iter_is_bvec(i)) {		\
			[...]					\
  -			iterate_bvec(i, n, base, len, off,	\
  -						bvec, (K))	\
  -			i->nr_segs -= bvec - i->bvec;		\
  -			i->bvec = bvec;				\
  -		} else if (iov_iter_is_kvec(i)) {		\
			[...]					\
  -			iterate_iovec(i, n, base, len, off,	\
  -						kvec, (K))	\
			[...]					\
  -		} else if (iov_iter_is_xarray(i)) {		\
			[...]					\
  -			iterate_xarray(i, n, base, len, off,	\
  -							(K))	\
  -		}						\
  -		i->count -= n;					\
  -	}							\
  -}
  -#define iterate_and_advance(i, n, base, len, off, I, K) \
  -	__iterate_and_advance(i, n, base, len, off, I, ((void)(K),0))
-------------------------------------------------------------------

Maybe we're all gonna fix it back? as follows：
-------------------------------------------------------------------
  --- a/include/linux/iov_iter.h
  +++ b/include/linux/iov_iter.h
  @@ -246,11 +246,11 @@ size_t iterate_and_advance2(struct iov_iter 
*iter, size_t len, void *priv,
          if (likely(iter_is_iovec(iter)))
                  return iterate_iovec(iter, len, priv, priv2, ustep);
          if (iov_iter_is_bvec(iter))
  -               return iterate_bvec(iter, len, priv, priv2, step);
  +               return iterate_bvec(iter, len, priv, priv2, ((void 
*)step, 0));
          if (iov_iter_is_kvec(iter))
  -               return iterate_kvec(iter, len, priv, priv2, step);
  +               return iterate_kvec(iter, len, priv, priv2, ((void 
*)step, 0));
          if (iov_iter_is_xarray(iter))
  -               return iterate_xarray(iter, len, priv, priv2, step);
  +               return iterate_xarray(iter, len, priv, priv2, ((void 
*)step, 0));
          return iterate_discard(iter, len, priv, priv2, step);
   }

  diff --git a/lib/iov_iter.c b/lib/iov_iter.c
  index e0aa6b440ca5..fabd5b1b97c7 100644
  --- a/lib/iov_iter.c
  +++ b/lib/iov_iter.c
  @@ -257,7 +257,7 @@ static size_t __copy_from_iter_mc(void *addr, 
size_t bytes, struct iov_iter *i)
                  bytes = i->count;
          if (unlikely(!bytes))
                  return 0;
  -       return iterate_bvec(i, bytes, addr, NULL, memcpy_from_iter_mc);
  +       return iterate_bvec(i, bytes, addr, NULL, ((void 
*)memcpy_from_iter_mc, 0));
   }

   static __always_inline
-------------------------------------------------------------------

   Hi, maintainer Alexander, what do you think ? :)

Thanks,
Tong.