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@xxxxxxx> --- 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
