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]

Message-ID: <20210322170916.GS1719932@casper.infradead.org>
Date:   Mon, 22 Mar 2021 17:09:16 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Sergey Senozhatsky <senozhatsky@...omium.org>
Cc:     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,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steve French <stfrench@...rosoft.com>
Subject: Re: [PATCH 3/5] cifsd: add file operations

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 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.

That said, there are plenty of opportunities to optimise the vmalloc code,
and that's worth pursuing.  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);