Re: [PATCH v3 8/8] cifs: use MAX_CIFS_SMALL_BUFFER_SIZE-8 as padding buffer

[Enzo Matsumiya <ematsumiya@suse.de>]

On 09/29, Stefan Metzmacher wrote:
>Am 29.09.22 um 03:56 schrieb Enzo Matsumiya:
>>AES-GMAC is more picky about buffers locality, alignment, and size, so
>>we can't use a stack-allocated buffer as padding (smb2_padding).
>>
>>This commit drops smb2_padding and "reserves" the 8 last bytes of each
>>small buffer, which are slab-allocated, as the padding buffer space.
>>
>>Introduce SMB2_PADDING_BUF(buf) macro to easily grab the padding buffer.
>>For now, only used in smb2_set_next_command().
>>
>>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
>>---
>>  fs/cifs/smb2ops.c | 7 +++++--
>>  fs/cifs/smb2pdu.c | 9 +++++----
>>  fs/cifs/smb2pdu.h | 2 --
>>  3 files changed, 10 insertions(+), 8 deletions(-)
>>
>>diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>index 22b40d181bba..0b8497e1c747 100644
>>--- a/fs/cifs/smb2ops.c
>>+++ b/fs/cifs/smb2ops.c
>>@@ -2323,7 +2323,10 @@ smb2_set_related(struct smb_rqst *rqst)
>>  	shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS;
>>  }
>>-char smb2_padding[7] = {0, 0, 0, 0, 0, 0, 0};
>>+/*
>>+ * Use the last 8 bytes of the small buf as the padding buffer, when necessary
>>+ */
>>+#define SMB2_PADDING_BUF(buf) (buf + MAX_CIFS_SMALL_BUFFER_SIZE - 8)
>
>Do we need to expend the size of MAX_CIFS_SMALL_BUFFER_SIZE
>in order to avoid reusing parts of the buffer used otherwise.
>
>(But MAX_CIFS_SMALL_BUFFER_SIZE is confusing magic I don't really
>understand, for me it's really hard to prove we never overflow
>MAX_CIFS_SMALL_BUFFER_SIZE).

Yes, that was one concern I had.  This is (yet another) part of the code
that I see where using hardcoded values hides their meanings and
reasoning, and the comments doesn't really help much understanding it.

So, based on my tests, that region (last 8 bytes) seems to not be ever used.
I didn't bother checking how much is actually being used, but for the
moment I'd say this is fine.

@Steve your clarification on the value for MAX_CIFS_SMALL_BUFFER_SIZE
would be appreciated, I guess.

>
>>  void
>>  smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst)
>>@@ -2352,7 +2355,7 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst)
>>  		 * If we do not have encryption then we can just add an extra
>>  		 * iov for the padding.
>>  		 */
>>-		rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding;
>>+		rqst->rq_iov[rqst->rq_nvec].iov_base = SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base);
>>  		rqst->rq_iov[rqst->rq_nvec].iov_len = num_padding;
>>  		rqst->rq_nvec++;
>>  		len += num_padding;
>>diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>>index 6c22ead51feb..fca1b580d57d 100644
>>--- a/fs/cifs/smb2pdu.c
>>+++ b/fs/cifs/smb2pdu.c
>>@@ -362,6 +362,9 @@ fill_small_buf(__le16 smb2_command, struct cifs_tcon *tcon,
>>  	/*
>>  	 * smaller than SMALL_BUFFER_SIZE but bigger than fixed area of
>>  	 * largest operations (Create)
>>+	 *
>>+	 * Note that the last 8 bytes of the small buffer are reserved for padding when required
>>+	 * (see SMB2_PADDING_BUF in smb2ops.c)
>>  	 */
>>  	memset(buf, 0, 256);
>>@@ -2993,8 +2996,7 @@ SMB2_open_free(struct smb_rqst *rqst)
>>  	if (rqst && rqst->rq_iov) {
>>  		cifs_small_buf_release(rqst->rq_iov[0].iov_base);
>>  		for (i = 1; i < rqst->rq_nvec; i++)
>>-			if (rqst->rq_iov[i].iov_base != smb2_padding)
>>-				kfree(rqst->rq_iov[i].iov_base);
>>+			kfree(rqst->rq_iov[i].iov_base);
>>  	}
>>  }
>>@@ -3187,8 +3189,7 @@ SMB2_ioctl_free(struct smb_rqst *rqst)
>>  	if (rqst && rqst->rq_iov) {
>>  		cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request */
>>  		for (i = 1; i < rqst->rq_nvec; i++)
>>-			if (rqst->rq_iov[i].iov_base != smb2_padding)
>>-				kfree(rqst->rq_iov[i].iov_base);
>>+			kfree(rqst->rq_iov[i].iov_base);
>
>Don't we need to check against SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base) here
>and avoid passing an invalid pointer to kfree()?

Ah good catch.  Will fix it.


>metze

Cheers,

Enzo