linux-kernel - Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible pages

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]

Message-ID: <42cda753-a5df-9024-9076-11d75eec16c1@intel.com>
Date:   Tue, 28 Apr 2020 17:25:16 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        akpm@...ux-foundation.org, jack@...e.cz, kirill@...temov.name
Cc:     borntraeger@...ibm.com, david@...hat.com, aarcange@...hat.com,
        linux-mm@...ck.org, frankja@...ux.ibm.com, sfr@...b.auug.org.au,
        jhubbard@...dia.com, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, peterz@...radead.org,
        sean.j.christopherson@...el.com
Subject: Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible
 pages

On 4/28/20 3:50 PM, Claudio Imbrenda wrote:
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -106,6 +106,9 @@ static int page_cache_pipe_buf_confirm(struct pipe_inode_info *pipe,
>  	struct page *page = buf->page;
>  	int err;
>  
> +	if (arch_make_page_accessible(page))
> +		return -EIO;
> +
>  	if (!PageUptodate(page)) {
>  		lock_page(page);

This is a cute fix, but doesn't it 100% depend on the internal
implementation detail of page cache sendfile() being implemented with a
pipe?  Depending on that seems rather fragile.  While I'm glad that you
surgically plugged the one single, specific case that I pointed out, I
can't help but suspect there are more of these.

For instance, I tried a file-to-file sendfile, basically:

	fd1 = open("file1");
	fd2 = open("file2");
	sendfile(fd1, fd2, ...);

ftrace showed page_cache_pipe_buf_confirm() getting called for the
source pipe pages but not the receiver.  There were no calls to
arch_make_page_accessible() outside of page_cache_pipe_buf_confirm() (I
put a stub in for it on x86 so I could trace it).

That indicates to me that one side of this might be fixed (the sender),
but the receiver is not.

This also doesn't even have the maintainer of fs/splice.c on cc.  The
changelog about what this is trying to do probably also lacks enough
context to bring Al up to speed about what this is trying to do.