| Message ID | 20210625135834.12934-3-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ceph+fscrypt: context, filename and symlink support | expand |
Some nits about comments: On Fri, Jun 25, 2021 at 09:58:12AM -0400, Jeff Layton wrote: > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > index 6ca7d16593ff..32b1f50433ba 100644 > --- a/fs/crypto/fname.c > +++ b/fs/crypto/fname.c > @@ -178,10 +178,8 @@ static int fname_decrypt(const struct inode *inode, > static const char lookup_table[65] = > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; > > -#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) > - > /** > - * base64_encode() - base64-encode some bytes > + * fscrypt_base64_encode() - base64-encode some bytes > * @src: the bytes to encode > * @len: number of bytes to encode > * @dst: (output) the base64-encoded string. Not NUL-terminated. > * > * Encodes the input string using characters from the set [A-Za-z0-9+,]. > * The encoded string is roughly 4/3 times the size of the input string. > * > * Return: length of the encoded string > */ > -static int base64_encode(const u8 *src, int len, char *dst) > +int fscrypt_base64_encode(const u8 *src, int len, char *dst) As this function will be used more widely, this comment should be fixed to be more precise. "Roughly 4/3" isn't precise; it's actually exactly FSCRYPT_BASE64_CHARS(len), right? The following would be better: * Encode the input bytes using characters from the set [A-Za-z0-9+,]. * * Return: length of the encoded string. This will be equal to * FSCRYPT_BASE64_CHARS(len). > +/** > + * fscrypt_base64_decode() - base64-decode some bytes > + * @src: the bytes to decode > + * @len: number of bytes to decode > + * @dst: (output) decoded binary data It's a bit confusing to talk about decoding "bytes"; it's really a string. How about: * fscrypt_base64_decode() - base64-decode a string * @src: the string to decode * @len: length of the source string, in bytes * @dst: (output) decoded binary data * * Decode a string that was previously encoded using fscrypt_base64_encode(). * The string doesn't need to be NUL-terminated. > + * Return: length of the decoded binary data Also the error return values should be documented, e.g.: * Return: length of the decoded binary data, or a negative number if the source * string isn't a valid base64-encoded string. - Eric
On Sun, 2021-07-11 at 12:40 -0500, Eric Biggers wrote: > Some nits about comments: > > On Fri, Jun 25, 2021 at 09:58:12AM -0400, Jeff Layton wrote: > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > > index 6ca7d16593ff..32b1f50433ba 100644 > > --- a/fs/crypto/fname.c > > +++ b/fs/crypto/fname.c > > @@ -178,10 +178,8 @@ static int fname_decrypt(const struct inode *inode, > > static const char lookup_table[65] = > > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; > > > > -#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) > > - > > /** > > - * base64_encode() - base64-encode some bytes > > + * fscrypt_base64_encode() - base64-encode some bytes > > * @src: the bytes to encode > > * @len: number of bytes to encode > > * @dst: (output) the base64-encoded string. Not NUL-terminated. > > * > > * Encodes the input string using characters from the set [A-Za-z0-9+,]. > > * The encoded string is roughly 4/3 times the size of the input string. > > * > > * Return: length of the encoded string > > */ > > -static int base64_encode(const u8 *src, int len, char *dst) > > +int fscrypt_base64_encode(const u8 *src, int len, char *dst) > > As this function will be used more widely, this comment should be fixed to be > more precise. "Roughly 4/3" isn't precise; it's actually exactly > FSCRYPT_BASE64_CHARS(len), right? The following would be better: > > * Encode the input bytes using characters from the set [A-Za-z0-9+,]. > * > * Return: length of the encoded string. This will be equal to > * FSCRYPT_BASE64_CHARS(len). > I'm not certain, but I thought that FSCRYPT_BASE64_CHARS gave you a worst-case estimate of the inflation. This returns the actual length of the resulting encoded string, which may be less than FSCRYPT_BASE64_CHARS(len). > > +/** > > + * fscrypt_base64_decode() - base64-decode some bytes > > + * @src: the bytes to decode > > + * @len: number of bytes to decode > > + * @dst: (output) decoded binary data > > It's a bit confusing to talk about decoding "bytes"; it's really a string. > How about: > > * fscrypt_base64_decode() - base64-decode a string > * @src: the string to decode > * @len: length of the source string, in bytes > * @dst: (output) decoded binary data > * > * Decode a string that was previously encoded using fscrypt_base64_encode(). > * The string doesn't need to be NUL-terminated. > > > + * Return: length of the decoded binary data > > Also the error return values should be documented, e.g.: > > * Return: length of the decoded binary data, or a negative number if the source > * string isn't a valid base64-encoded string. > That update looks reasonable. Thanks,
On Mon, Jul 12, 2021 at 07:55:37AM -0400, Jeff Layton wrote: > On Sun, 2021-07-11 at 12:40 -0500, Eric Biggers wrote: > > Some nits about comments: > > > > On Fri, Jun 25, 2021 at 09:58:12AM -0400, Jeff Layton wrote: > > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > > > index 6ca7d16593ff..32b1f50433ba 100644 > > > --- a/fs/crypto/fname.c > > > +++ b/fs/crypto/fname.c > > > @@ -178,10 +178,8 @@ static int fname_decrypt(const struct inode *inode, > > > static const char lookup_table[65] = > > > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; > > > > > > -#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) > > > - > > > /** > > > - * base64_encode() - base64-encode some bytes > > > + * fscrypt_base64_encode() - base64-encode some bytes > > > * @src: the bytes to encode > > > * @len: number of bytes to encode > > > * @dst: (output) the base64-encoded string. Not NUL-terminated. > > > * > > > * Encodes the input string using characters from the set [A-Za-z0-9+,]. > > > * The encoded string is roughly 4/3 times the size of the input string. > > > * > > > * Return: length of the encoded string > > > */ > > > -static int base64_encode(const u8 *src, int len, char *dst) > > > +int fscrypt_base64_encode(const u8 *src, int len, char *dst) > > > > As this function will be used more widely, this comment should be fixed to be > > more precise. "Roughly 4/3" isn't precise; it's actually exactly > > FSCRYPT_BASE64_CHARS(len), right? The following would be better: > > > > * Encode the input bytes using characters from the set [A-Za-z0-9+,]. > > * > > * Return: length of the encoded string. This will be equal to > > * FSCRYPT_BASE64_CHARS(len). > > > > I'm not certain, but I thought that FSCRYPT_BASE64_CHARS gave you a > worst-case estimate of the inflation. This returns the actual length of > the resulting encoded string, which may be less than > FSCRYPT_BASE64_CHARS(len). > As far as I can tell, it's the exact amount. - Eric
On Mon, 2021-07-12 at 09:22 -0500, Eric Biggers wrote: > On Mon, Jul 12, 2021 at 07:55:37AM -0400, Jeff Layton wrote: > > On Sun, 2021-07-11 at 12:40 -0500, Eric Biggers wrote: > > > Some nits about comments: > > > > > > On Fri, Jun 25, 2021 at 09:58:12AM -0400, Jeff Layton wrote: > > > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > > > > index 6ca7d16593ff..32b1f50433ba 100644 > > > > --- a/fs/crypto/fname.c > > > > +++ b/fs/crypto/fname.c > > > > @@ -178,10 +178,8 @@ static int fname_decrypt(const struct inode *inode, > > > > static const char lookup_table[65] = > > > > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; > > > > > > > > -#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) > > > > - > > > > /** > > > > - * base64_encode() - base64-encode some bytes > > > > + * fscrypt_base64_encode() - base64-encode some bytes > > > > * @src: the bytes to encode > > > > * @len: number of bytes to encode > > > > * @dst: (output) the base64-encoded string. Not NUL-terminated. > > > > * > > > > * Encodes the input string using characters from the set [A-Za-z0-9+,]. > > > > * The encoded string is roughly 4/3 times the size of the input string. > > > > * > > > > * Return: length of the encoded string > > > > */ > > > > -static int base64_encode(const u8 *src, int len, char *dst) > > > > +int fscrypt_base64_encode(const u8 *src, int len, char *dst) > > > > > > As this function will be used more widely, this comment should be fixed to be > > > more precise. "Roughly 4/3" isn't precise; it's actually exactly > > > FSCRYPT_BASE64_CHARS(len), right? The following would be better: > > > > > > * Encode the input bytes using characters from the set [A-Za-z0-9+,]. > > > * > > > * Return: length of the encoded string. This will be equal to > > > * FSCRYPT_BASE64_CHARS(len). > > > > > > > I'm not certain, but I thought that FSCRYPT_BASE64_CHARS gave you a > > worst-case estimate of the inflation. This returns the actual length of > > the resulting encoded string, which may be less than > > FSCRYPT_BASE64_CHARS(len). > > > > As far as I can tell, it's the exact amount. Yeah, now that I went back and re-read the code, I think you're right. I'll fix the comment. Thanks,
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 6ca7d16593ff..32b1f50433ba 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -178,10 +178,8 @@ static int fname_decrypt(const struct inode *inode, static const char lookup_table[65] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; -#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) - /** - * base64_encode() - base64-encode some bytes + * fscrypt_base64_encode() - base64-encode some bytes * @src: the bytes to encode * @len: number of bytes to encode * @dst: (output) the base64-encoded string. Not NUL-terminated. @@ -191,7 +189,7 @@ static const char lookup_table[65] = * * Return: length of the encoded string */ -static int base64_encode(const u8 *src, int len, char *dst) +int fscrypt_base64_encode(const u8 *src, int len, char *dst) { int i, bits = 0, ac = 0; char *cp = dst; @@ -209,8 +207,20 @@ static int base64_encode(const u8 *src, int len, char *dst) *cp++ = lookup_table[ac & 0x3f]; return cp - dst; } +EXPORT_SYMBOL(fscrypt_base64_encode); -static int base64_decode(const char *src, int len, u8 *dst) +/** + * fscrypt_base64_decode() - base64-decode some bytes + * @src: the bytes to decode + * @len: number of bytes to decode + * @dst: (output) decoded binary data + * + * Decode an input string that was previously encoded using + * fscrypt_base64_encode. + * + * Return: length of the decoded binary data + */ +int fscrypt_base64_decode(const char *src, int len, u8 *dst) { int i, bits = 0, ac = 0; const char *p; @@ -232,6 +242,7 @@ static int base64_decode(const char *src, int len, u8 *dst) return -1; return cp - dst; } +EXPORT_SYMBOL(fscrypt_base64_decode); bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy, u32 orig_len, u32 max_len, @@ -263,8 +274,9 @@ bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy, int fscrypt_fname_alloc_buffer(u32 max_encrypted_len, struct fscrypt_str *crypto_str) { - const u32 max_encoded_len = BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX); u32 max_presented_len; + const u32 max_encoded_len = + FSCRYPT_BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX); max_presented_len = max(max_encoded_len, max_encrypted_len); @@ -342,7 +354,7 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode, offsetof(struct fscrypt_nokey_name, bytes)); BUILD_BUG_ON(offsetofend(struct fscrypt_nokey_name, bytes) != offsetof(struct fscrypt_nokey_name, sha256)); - BUILD_BUG_ON(BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX); + BUILD_BUG_ON(FSCRYPT_BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX) > NAME_MAX); if (hash) { nokey_name.dirhash[0] = hash; @@ -362,7 +374,8 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode, nokey_name.sha256); size = FSCRYPT_NOKEY_NAME_MAX; } - oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name); + oname->len = fscrypt_base64_encode((const u8 *)&nokey_name, size, + oname->name); return 0; } EXPORT_SYMBOL(fscrypt_fname_disk_to_usr); @@ -436,14 +449,15 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname, * user-supplied name */ - if (iname->len > BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX)) + if (iname->len > FSCRYPT_BASE64_CHARS(FSCRYPT_NOKEY_NAME_MAX)) return -ENOENT; fname->crypto_buf.name = kmalloc(FSCRYPT_NOKEY_NAME_MAX, GFP_KERNEL); if (fname->crypto_buf.name == NULL) return -ENOMEM; - ret = base64_decode(iname->name, iname->len, fname->crypto_buf.name); + ret = fscrypt_base64_decode(iname->name, iname->len, + fname->crypto_buf.name); if (ret < (int)offsetof(struct fscrypt_nokey_name, bytes[1]) || (ret > offsetof(struct fscrypt_nokey_name, sha256) && ret != FSCRYPT_NOKEY_NAME_MAX)) { diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 2ea1387bb497..e300f6145ddc 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -46,6 +46,9 @@ struct fscrypt_name { /* Maximum value for the third parameter of fscrypt_operations.set_context(). */ #define FSCRYPT_SET_CONTEXT_MAX_SIZE 40 +/* Calculate worst-case base64 encoding inflation */ +#define FSCRYPT_BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) + #ifdef CONFIG_FS_ENCRYPTION /* * fscrypt superblock flags @@ -207,6 +210,8 @@ void fscrypt_free_inode(struct inode *inode); int fscrypt_drop_inode(struct inode *inode); /* fname.c */ +int fscrypt_base64_encode(const u8 *src, int len, char *dst); +int fscrypt_base64_decode(const char *src, int len, u8 *dst); int fscrypt_setup_filename(struct inode *inode, const struct qstr *iname, int lookup, struct fscrypt_name *fname);
Ceph is going to add fscrypt support, but we still want encrypted filenames to be composed of printable characters, so we can maintain compatibility with clients that don't support fscrypt. We could just adopt fscrypt's current nokey name format, but that is subject to change in the future, and it also contains dirhash fields that we don't need for cephfs. Because of this, we're going to concoct our own scheme for encoding encrypted filenames. It's very similar to fscrypt's current scheme, but doesn't bother with the dirhash fields. The ceph encoding scheme will use base64 encoding as well, and we also want it to avoid characters that are illegal in filenames. Export the fscrypt base64 encoding/decoding routines so we can use them in ceph's fscrypt implementation. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/crypto/fname.c | 34 ++++++++++++++++++++++++---------- include/linux/fscrypt.h | 5 +++++ 2 files changed, 29 insertions(+), 10 deletions(-)