netdev - Re: [bug report] dead loop in generic_perform_write() //Re: [PATCH v7 07/12] iov

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 29 Feb 2024 09:32:31 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Tong Tiangen <tongtiangen@...wei.com>, Al Viro <viro@...nel.org>
Cc: David Howells <dhowells@...hat.com>, Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>, 
	Christian Brauner <christian@...uner.io>, David Laight <David.Laight@...lab.com>, 
	Matthew Wilcox <willy@...radead.org>, Jeff Layton <jlayton@...nel.org>, linux-fsdevel@...r.kernel.org, 
	linux-block@...r.kernel.org, linux-mm@...ck.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [bug report] dead loop in generic_perform_write() //Re: [PATCH v7
 07/12] iov_iter: Convert iterate*() to inline funcs

On Thu, 29 Feb 2024 at 00:13, Tong Tiangen <tongtiangen@...wei.com> wrote:
>
> See the logic before this patch, always success (((void)(K),0)) is
> returned for three types: ITER_BVEC, ITER_KVEC and ITER_XARRAY.

No, look closer.

Yes, the iterate_and_advance() macro does that "((void)(K),0)" to make
the compiler generate better code for those cases (because then the
compiler can see that the return value is a compile-time zero), but
notice how _copy_mc_to_iter() didn't use that macro back then. It used
the unvarnished __iterate_and_advance() exactly so that the MC copy
case would *not* get that "always return zero" behavior.

That goes back to (in a different form) at least commit 1b4fb5ffd79b
("iov_iter: teach iterate_{bvec,xarray}() about possible short
copies").

But hardly anybody ever tests this machine-check special case code, so
who knows when it broke again.

I'm just looking at the source code, and with all the macro games it's
*really* hard to follow, so I may well be missing something.

> Maybe we're all gonna fix it back? as follows：

No. We could do it for the kvec and xarray case, just to get better
code generation again (not that I looked at it, so who knows), but the
one case that actually uses memcpy_from_iter_mc() needs to react to a
short write.

One option might be to make a failed memcpy_from_iter_mc() set another
flag in the iter, and then make fault_in_iov_iter_readable() test that
flag and return 'len' if that flag is set.

Something like that (wild handwaving) should get the right error handling.

The simpler alternative is maybe something like the attached.
COMPLETELY UNTESTED. Maybe I've confused myself with all the different
indiraction mazes in the iov_iter code.

                     Linus

View attachment "patch.diff" of type "text/x-patch" (618 bytes)