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

[Tong Tiangen <tongtiangen@huawei.com>]

在 2024/2/29 6:57, Linus Torvalds 写道:
> On Wed, 28 Feb 2024 at 13:21, Linus Torvalds
> <torvalds@linux-foundation.org> 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.