linux-kernel - Re: [PATCH 3/5] cifsd: add file operations

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:   Tue, 23 Mar 2021 09:05:51 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Namjae Jeon <namjae.jeon@...sung.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-cifs@...r.kernel.org,
        linux-cifsd-devel@...ts.sourceforge.net, smfrench@...il.com,
        hyc.lee@...il.com, viro@...iv.linux.org.uk, hch@....de,
        hch@...radead.org, ronniesahlberg@...il.com,
        aurelien.aptel@...il.com, aaptel@...e.com, sandeen@...deen.net,
        dan.carpenter@...cle.com, colin.king@...onical.com,
        rdunlap@...radead.org, Steve French <stfrench@...rosoft.com>
Subject: Re: [PATCH 3/5] cifsd: add file operations

On (21/03/22 17:09), Matthew Wilcox wrote:
> On Mon, Mar 22, 2021 at 06:03:21PM +0900, Sergey Senozhatsky wrote:
> > On (21/03/22 08:15), Matthew Wilcox wrote:
> > > 
> > > What's the scenario for which your allocator performs better than slub
> > > 
> > 
> > IIRC request and reply buffers can be up to 4M in size. So this stuff
> > just allocates a number of fat buffers and keeps them around so that
> > it doesn't have to vmalloc(4M) for every request and every response.
> 
> Hang on a minute, this speaks to a deeper design problem.  If we're doing
> a 'request' or 'reply' that is that large, the I/O should be coming from
> or going to the page cache.  If it goes via a 4MB virtually contiguous
> buffer first, that's a memcpy that could/should be avoided.

But huge vmalloc buffers are still needed. For instance, `ls -la` in
a directory with a huge number of entries.

> But now i'm looking for how ksmbd_find_buffer() is used, and it isn't.
> So it looks like someone came to the same conclusion I did, but forgot
> to delete the wm code.

Yes, I think it's disabled by default and requires some smb.conf
configuration. So I guess that wm code can be removed. Especially given
that

> That said, there are plenty of opportunities to optimise the vmalloc code,
> and that's worth pursuing.

That would be really interesting to see!

> And here's the receive path which contains
> the memcpy that should be avoided (ok, it's actually the call to ->read;
> we shouldn't be reading in the entire 4MB but rather the header):

> +		conn->request_buf = ksmbd_alloc_request(size);
> +		if (!conn->request_buf)
> +			continue;
> +
> +		memcpy(conn->request_buf, hdr_buf, sizeof(hdr_buf));
> +		if (!ksmbd_smb_request(conn))
> +			break;
> +
> +		/*
> +		 * We already read 4 bytes to find out PDU size, now
> +		 * read in PDU
> +		 */
> +		size = t->ops->read(t, conn->request_buf + 4, pdu_size);


// A side note, it seems that the maximum read/write/trans buffer size that
// windows supports is 8MB, not 4MB.