* Re: [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-24 9:14 [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic Yunlong Song
@ 2018-02-24 18:32 ` Eric Biggers
2018-02-26 3:06 ` Yunlong Song
2018-02-26 3:42 ` Chao Yu
2018-02-26 2:57 ` [PATCH v2] " Yunlong Song
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Eric Biggers @ 2018-02-24 18:32 UTC (permalink / raw)
To: Yunlong Song
Cc: jaegeuk, chao, yuchao0, yunlong.song, miaoxie, bintian.wang,
shengyong1, heyunlei, linux-f2fs-devel, linux-kernel
Hi Yunlong,
On Sat, Feb 24, 2018 at 05:14:58PM +0800, Yunlong Song wrote:
> In some platforms (such as arm), high memory is used, then the
> decrypting filename will cause panic, the reason see commit
> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
> for decrypting filename"):
>
> We got dentry pages from high_mem, and its address space directly goes into the
> decryption path via f2fs_fname_disk_to_usr.
> But, sg_init_one assumes the address is not from high_mem, so we can get this
> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>
> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>
> Howerver, later patches:
> commit 922ec355f86365388203672119b5bca346a45085 ("f2fs crypto: avoid
> unneeded memory allocation when {en/de}crypting symlink")
> commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
> unneeded memory allocation in ->readdir")
>
> reverts the codes, which causes panic again in arm, so let's add the old
> patch again.
>
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
> fs/f2fs/dir.c | 7 +++++++
> fs/f2fs/namei.c | 10 +++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index f00b5ed..c0cf3e7b 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
> int save_len = fstr->len;
> int err;
>
> + de_name.name = kmalloc(de_name.len, GFP_NOFS);
> + if (!de_name.name)
> + return -ENOMEM;
> +
> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
> +
> err = fscrypt_fname_disk_to_usr(d->inode,
> (u32)de->hash_code, 0,
> &de_name, fstr);
> + kfree(de_name.name);
> if (err)
> return err;
>
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index c4c94c7..2cb70c1 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -1170,8 +1170,13 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>
> /* Symlink is encrypted */
> sd = (struct fscrypt_symlink_data *)caddr;
> - cstr.name = sd->encrypted_path;
> cstr.len = le16_to_cpu(sd->len);
> + cstr.name = kmalloc(cstr.len, GFP_NOFS);
> + if (!cstr.name) {
> + res = -ENOMEM;
> + goto errout;
> + }
> + memcpy(cstr.name, sd->encrypted_path, cstr.len);
>
> /* this is broken symlink case */
> if (unlikely(cstr.len == 0)) {
> @@ -1198,6 +1203,8 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> goto errout;
> }
>
> + kfree(cstr.name);
> +
> paddr = pstr.name;
>
> /* Null-terminate the name */
> @@ -1207,6 +1214,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
> set_delayed_call(done, kfree_link, paddr);
> return paddr;
> errout:
> + kfree(cstr.name);
> fscrypt_fname_free_buffer(&pstr);
> put_page(cpage);
> return ERR_PTR(res);
> --
> 1.8.5.2
>
The pagecache for symlinks in f2fs already uses lowmem only, so the change to
->get_link() isn't needed. Also that part of the patch doesn't apply to
upstream.
For directories we may need your fix. Note: kmalloc + memcpy should be replaced
with kmemdup. But alternatively, directory pages could be restricted to lowmem
which would match ext4.
Eric
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-24 18:32 ` Eric Biggers
@ 2018-02-26 3:06 ` Yunlong Song
2018-02-26 3:42 ` Chao Yu
1 sibling, 0 replies; 14+ messages in thread
From: Yunlong Song @ 2018-02-26 3:06 UTC (permalink / raw)
To: Eric Biggers
Cc: jaegeuk, chao, yuchao0, yunlong.song, miaoxie, bintian.wang,
shengyong1, heyunlei, linux-f2fs-devel, linux-kernel
Hi, Eric,
Thanks for your review, I have removed the symlink part and use
kmemdup instead. As for directory pages restricted to lowmem, I am
not sure whether it is proper for f2fs, since the directory structures
of f2fs are different from ext4. So I just keep its old kmap.
On 2018/2/25 2:32, Eric Biggers wrote:
> Hi Yunlong,
>
> On Sat, Feb 24, 2018 at 05:14:58PM +0800, Yunlong Song wrote:
>> In some platforms (such as arm), high memory is used, then the
>> decrypting filename will cause panic, the reason see commit
>> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
>> for decrypting filename"):
>>
>> We got dentry pages from high_mem, and its address space directly goes into the
>> decryption path via f2fs_fname_disk_to_usr.
>> But, sg_init_one assumes the address is not from high_mem, so we can get this
>> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>>
>> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>> ...
>> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
>> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
>> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
>> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
>> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
>> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
>> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
>> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
>> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
>> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
>> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>>
>> Howerver, later patches:
>> commit 922ec355f86365388203672119b5bca346a45085 ("f2fs crypto: avoid
>> unneeded memory allocation when {en/de}crypting symlink")
>> commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
>> unneeded memory allocation in ->readdir")
>>
>> reverts the codes, which causes panic again in arm, so let's add the old
>> patch again.
>>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>> fs/f2fs/dir.c | 7 +++++++
>> fs/f2fs/namei.c | 10 +++++++++-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index f00b5ed..c0cf3e7b 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>> int save_len = fstr->len;
>> int err;
>>
>> + de_name.name = kmalloc(de_name.len, GFP_NOFS);
>> + if (!de_name.name)
>> + return -ENOMEM;
>> +
>> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>> +
>> err = fscrypt_fname_disk_to_usr(d->inode,
>> (u32)de->hash_code, 0,
>> &de_name, fstr);
>> + kfree(de_name.name);
>> if (err)
>> return err;
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index c4c94c7..2cb70c1 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -1170,8 +1170,13 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>>
>> /* Symlink is encrypted */
>> sd = (struct fscrypt_symlink_data *)caddr;
>> - cstr.name = sd->encrypted_path;
>> cstr.len = le16_to_cpu(sd->len);
>> + cstr.name = kmalloc(cstr.len, GFP_NOFS);
>> + if (!cstr.name) {
>> + res = -ENOMEM;
>> + goto errout;
>> + }
>> + memcpy(cstr.name, sd->encrypted_path, cstr.len);
>>
>> /* this is broken symlink case */
>> if (unlikely(cstr.len == 0)) {
>> @@ -1198,6 +1203,8 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>> goto errout;
>> }
>>
>> + kfree(cstr.name);
>> +
>> paddr = pstr.name;
>>
>> /* Null-terminate the name */
>> @@ -1207,6 +1214,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>> set_delayed_call(done, kfree_link, paddr);
>> return paddr;
>> errout:
>> + kfree(cstr.name);
>> fscrypt_fname_free_buffer(&pstr);
>> put_page(cpage);
>> return ERR_PTR(res);
>> --
>> 1.8.5.2
>>
>
> The pagecache for symlinks in f2fs already uses lowmem only, so the change to
> ->get_link() isn't needed. Also that part of the patch doesn't apply to
> upstream.
>
> For directories we may need your fix. Note: kmalloc + memcpy should be replaced
> with kmemdup. But alternatively, directory pages could be restricted to lowmem
> which would match ext4.
>
> Eric
>
> .
>
--
Thanks,
Yunlong Song
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-24 18:32 ` Eric Biggers
2018-02-26 3:06 ` Yunlong Song
@ 2018-02-26 3:42 ` Chao Yu
1 sibling, 0 replies; 14+ messages in thread
From: Chao Yu @ 2018-02-26 3:42 UTC (permalink / raw)
To: Eric Biggers, Yunlong Song
Cc: jaegeuk, chao, yunlong.song, miaoxie, bintian.wang, shengyong1,
heyunlei, linux-f2fs-devel, linux-kernel
Hi Eric, Yunlong,
On 2018/2/25 2:32, Eric Biggers wrote:
> Hi Yunlong,
>
> On Sat, Feb 24, 2018 at 05:14:58PM +0800, Yunlong Song wrote:
>> In some platforms (such as arm), high memory is used, then the
>> decrypting filename will cause panic, the reason see commit
>> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
>> for decrypting filename"):
>>
>> We got dentry pages from high_mem, and its address space directly goes into the
>> decryption path via f2fs_fname_disk_to_usr.
>> But, sg_init_one assumes the address is not from high_mem, so we can get this
>> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>>
>> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>> ...
>> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
>> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
>> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
>> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
>> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
>> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
>> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
>> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
>> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
>> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
>> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>>
>> Howerver, later patches:
>> commit 922ec355f86365388203672119b5bca346a45085 ("f2fs crypto: avoid
>> unneeded memory allocation when {en/de}crypting symlink")
>> commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
>> unneeded memory allocation in ->readdir")
>>
>> reverts the codes, which causes panic again in arm, so let's add the old
>> patch again.
>>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>> fs/f2fs/dir.c | 7 +++++++
>> fs/f2fs/namei.c | 10 +++++++++-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index f00b5ed..c0cf3e7b 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>> int save_len = fstr->len;
>> int err;
>>
>> + de_name.name = kmalloc(de_name.len, GFP_NOFS);
>> + if (!de_name.name)
>> + return -ENOMEM;
>> +
>> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>> +
>> err = fscrypt_fname_disk_to_usr(d->inode,
>> (u32)de->hash_code, 0,
>> &de_name, fstr);
>> + kfree(de_name.name);
>> if (err)
>> return err;
>>
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index c4c94c7..2cb70c1 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -1170,8 +1170,13 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>>
>> /* Symlink is encrypted */
>> sd = (struct fscrypt_symlink_data *)caddr;
>> - cstr.name = sd->encrypted_path;
>> cstr.len = le16_to_cpu(sd->len);
>> + cstr.name = kmalloc(cstr.len, GFP_NOFS);
>> + if (!cstr.name) {
>> + res = -ENOMEM;
>> + goto errout;
>> + }
>> + memcpy(cstr.name, sd->encrypted_path, cstr.len);
>>
>> /* this is broken symlink case */
>> if (unlikely(cstr.len == 0)) {
>> @@ -1198,6 +1203,8 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>> goto errout;
>> }
>>
>> + kfree(cstr.name);
>> +
>> paddr = pstr.name;
>>
>> /* Null-terminate the name */
>> @@ -1207,6 +1214,7 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
>> set_delayed_call(done, kfree_link, paddr);
>> return paddr;
>> errout:
>> + kfree(cstr.name);
>> fscrypt_fname_free_buffer(&pstr);
>> put_page(cpage);
>> return ERR_PTR(res);
>> --
>> 1.8.5.2
>>
>
> The pagecache for symlinks in f2fs already uses lowmem only, so the change to
> ->get_link() isn't needed. Also that part of the patch doesn't apply to
> upstream.
>
> For directories we may need your fix. Note: kmalloc + memcpy should be replaced
> with kmemdup. But alternatively, directory pages could be restricted to lowmem
I'd like to suggest to use f2fs_kmalloc, so that we can deploy memory allocation
failure injection functionality in all places of f2fs, which can help to check
error handling in those places.
Thanks,
> which would match ext4.
>
> Eric
>
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-24 9:14 [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic Yunlong Song
2018-02-24 18:32 ` Eric Biggers
@ 2018-02-26 2:57 ` Yunlong Song
2018-02-27 10:40 ` Chao Yu
2018-02-28 2:19 ` [PATCH v3] " Yunlong Song
2018-02-28 3:17 ` [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir" Yunlong Song
3 siblings, 1 reply; 14+ messages in thread
From: Yunlong Song @ 2018-02-26 2:57 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In some platforms (such as arm), high memory is used, then the
decrypting filename will cause panic, the reason see commit
569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
for decrypting filename"):
We got dentry pages from high_mem, and its address space directly goes into the
decryption path via f2fs_fname_disk_to_usr.
But, sg_init_one assumes the address is not from high_mem, so we can get this
panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
...
(kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
(__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
(blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
(crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
(crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
(async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
(f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
(f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
(f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
(vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
(SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
Howerver, later patches:
commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
unneeded memory allocation in ->readdir")
reverts the codes, which causes panic again in arm, so let's add part of
the old patch again for dentry page.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/dir.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index f00b5ed..23fff48 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -825,9 +825,15 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
int save_len = fstr->len;
int err;
+ de_name.name = kmemdup(d->filename[bit_pos],
+ de_name.len, GFP_NOFS);
+ if (!de_name.name)
+ return -ENOMEM;
+
err = fscrypt_fname_disk_to_usr(d->inode,
(u32)de->hash_code, 0,
&de_name, fstr);
+ kfree(de_name.name);
if (err)
return err;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-26 2:57 ` [PATCH v2] " Yunlong Song
@ 2018-02-27 10:40 ` Chao Yu
0 siblings, 0 replies; 14+ messages in thread
From: Chao Yu @ 2018-02-27 10:40 UTC (permalink / raw)
To: Yunlong Song, jaegeuk, chao, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
On 2018/2/26 10:57, Yunlong Song wrote:
> In some platforms (such as arm), high memory is used, then the
> decrypting filename will cause panic, the reason see commit
> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
> for decrypting filename"):
>
> We got dentry pages from high_mem, and its address space directly goes into the
> decryption path via f2fs_fname_disk_to_usr.
> But, sg_init_one assumes the address is not from high_mem, so we can get this
> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>
> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>
> Howerver, later patches:
> commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
> unneeded memory allocation in ->readdir")
>
> reverts the codes, which causes panic again in arm, so let's add part of
> the old patch again for dentry page.
>
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
> fs/f2fs/dir.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index f00b5ed..23fff48 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -825,9 +825,15 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
> int save_len = fstr->len;
> int err;
>
> + de_name.name = kmemdup(d->filename[bit_pos],
How about using f2fs_kmalloc + memcpy here?
Thanks,
> + de_name.len, GFP_NOFS);
> + if (!de_name.name)
> + return -ENOMEM;
> +
> err = fscrypt_fname_disk_to_usr(d->inode,
> (u32)de->hash_code, 0,
> &de_name, fstr);
> + kfree(de_name.name);
> if (err)
> return err;
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-24 9:14 [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic Yunlong Song
2018-02-24 18:32 ` Eric Biggers
2018-02-26 2:57 ` [PATCH v2] " Yunlong Song
@ 2018-02-28 2:19 ` Yunlong Song
2018-02-28 2:49 ` Chao Yu
2018-02-28 3:17 ` [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir" Yunlong Song
3 siblings, 1 reply; 14+ messages in thread
From: Yunlong Song @ 2018-02-28 2:19 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In some platforms (such as arm), high memory is used, then the
decrypting filename will cause panic, the reason see commit
569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
for decrypting filename"):
We got dentry pages from high_mem, and its address space directly goes into the
decryption path via f2fs_fname_disk_to_usr.
But, sg_init_one assumes the address is not from high_mem, so we can get this
panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
...
(kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
(__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
(blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
(crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
(crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
(async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
(f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
(f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
(f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
(vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
(SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
Howerver, later patches:
commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
unneeded memory allocation in ->readdir")
reverts the codes, which causes panic again in arm, so let's add part of
the old patch again for dentry page.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/dir.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index f00b5ed..de2e295 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
int save_len = fstr->len;
int err;
+ de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
+ if (!de_name.name)
+ return -ENOMEM;
+
+ memcpy(de_name.name, d->filename[bit_pos], de_name.len);
+
err = fscrypt_fname_disk_to_usr(d->inode,
(u32)de->hash_code, 0,
&de_name, fstr);
+ kfree(de_name.name);
if (err)
return err;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3] f2fs: allocate buffer for decrypting filename to avoid panic
2018-02-28 2:19 ` [PATCH v3] " Yunlong Song
@ 2018-02-28 2:49 ` Chao Yu
0 siblings, 0 replies; 14+ messages in thread
From: Chao Yu @ 2018-02-28 2:49 UTC (permalink / raw)
To: Yunlong Song, jaegeuk, chao, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
On 2018/2/28 10:19, Yunlong Song wrote:
> In some platforms (such as arm), high memory is used, then the
> decrypting filename will cause panic, the reason see commit
> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
> for decrypting filename"):
>
> We got dentry pages from high_mem, and its address space directly goes into the
> decryption path via f2fs_fname_disk_to_usr.
> But, sg_init_one assumes the address is not from high_mem, so we can get this
> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>
> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>
> Howerver, later patches:
> commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b ("f2fs crypto: avoid
> unneeded memory allocation in ->readdir")
>
> reverts the codes, which causes panic again in arm, so let's add part of
> the old patch again for dentry page.
>
Fixes: e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
Looks good to me, you can add:
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Minor, we can just use 'git revert' to generate patch, so that commit title can
notice the patch are reverting buggy commit.
Thanks,
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-02-24 9:14 [PATCH] f2fs: allocate buffer for decrypting filename to avoid panic Yunlong Song
` (2 preceding siblings ...)
2018-02-28 2:19 ` [PATCH v3] " Yunlong Song
@ 2018-02-28 3:17 ` Yunlong Song
2018-02-28 5:48 ` Jaegeuk Kim
3 siblings, 1 reply; 14+ messages in thread
From: Yunlong Song @ 2018-02-28 3:17 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
This reverts commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b.
Conflicts:
fs/f2fs/dir.c
In some platforms (such as arm), high memory is used, then the
decrypting filename will cause panic, the reason see commit
569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
for decrypting filename"):
We got dentry pages from high_mem, and its address space directly goes into the
decryption path via f2fs_fname_disk_to_usr.
But, sg_init_one assumes the address is not from high_mem, so we can get this
panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
...
(kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
(__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
(blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
(crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
(crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
(async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
(f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
(f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
(f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
(vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
(SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
Howerver, later patch:
commit e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
reverts the codes, which causes panic again in arm, so fix it back to the old version.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
Reviewed-by: Chao Yu <yuchao0@huawei.com>
---
fs/f2fs/dir.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index f00b5ed..de2e295 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
int save_len = fstr->len;
int err;
+ de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
+ if (!de_name.name)
+ return -ENOMEM;
+
+ memcpy(de_name.name, d->filename[bit_pos], de_name.len);
+
err = fscrypt_fname_disk_to_usr(d->inode,
(u32)de->hash_code, 0,
&de_name, fstr);
+ kfree(de_name.name);
if (err)
return err;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-02-28 3:17 ` [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir" Yunlong Song
@ 2018-02-28 5:48 ` Jaegeuk Kim
2018-02-28 9:50 ` Chao Yu
2018-02-28 12:32 ` Yunlong Song
0 siblings, 2 replies; 14+ messages in thread
From: Jaegeuk Kim @ 2018-02-28 5:48 UTC (permalink / raw)
To: Yunlong Song
Cc: chao, yuchao0, yunlong.song, miaoxie, bintian.wang, shengyong1,
heyunlei, linux-f2fs-devel, linux-kernel
Hi Yunlong,
As Eric pointed out, how do you think using nohighmem for directory likewise
ext4, which looks like more efficient? Actually, we don't need to do this in
most of recent kernels, right?
Thanks,
On 02/28, Yunlong Song wrote:
> This reverts commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b.
>
> Conflicts:
> fs/f2fs/dir.c
>
> In some platforms (such as arm), high memory is used, then the
> decrypting filename will cause panic, the reason see commit
> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
> for decrypting filename"):
>
> We got dentry pages from high_mem, and its address space directly goes into the
> decryption path via f2fs_fname_disk_to_usr.
> But, sg_init_one assumes the address is not from high_mem, so we can get this
> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>
> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>
> Howerver, later patch:
> commit e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
> reverts the codes, which causes panic again in arm, so fix it back to the old version.
>
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> ---
> fs/f2fs/dir.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index f00b5ed..de2e295 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
> int save_len = fstr->len;
> int err;
>
> + de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
> + if (!de_name.name)
> + return -ENOMEM;
> +
> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
> +
> err = fscrypt_fname_disk_to_usr(d->inode,
> (u32)de->hash_code, 0,
> &de_name, fstr);
> + kfree(de_name.name);
> if (err)
> return err;
>
> --
> 1.8.5.2
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-02-28 5:48 ` Jaegeuk Kim
@ 2018-02-28 9:50 ` Chao Yu
2018-03-01 2:50 ` Jaegeuk Kim
2018-02-28 12:32 ` Yunlong Song
1 sibling, 1 reply; 14+ messages in thread
From: Chao Yu @ 2018-02-28 9:50 UTC (permalink / raw)
To: Jaegeuk Kim, Yunlong Song
Cc: chao, yunlong.song, miaoxie, bintian.wang, shengyong1, heyunlei,
linux-f2fs-devel, linux-kernel
Hi Jaegeuk,
On 2018/2/28 13:48, Jaegeuk Kim wrote:
> Hi Yunlong,
>
> As Eric pointed out, how do you think using nohighmem for directory likewise
I'd like to ask, at the beginning, why we choose to use highmem for dentry page?
any history reason there?
> ext4, which looks like more efficient? Actually, we don't need to do this in
> most of recent kernels, right?
It's OK to me to keep a line with ext4.
Thanks,
>
> Thanks,
>
> On 02/28, Yunlong Song wrote:
>> This reverts commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b.
>>
>> Conflicts:
>> fs/f2fs/dir.c
>>
>> In some platforms (such as arm), high memory is used, then the
>> decrypting filename will cause panic, the reason see commit
>> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
>> for decrypting filename"):
>>
>> We got dentry pages from high_mem, and its address space directly goes into the
>> decryption path via f2fs_fname_disk_to_usr.
>> But, sg_init_one assumes the address is not from high_mem, so we can get this
>> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>>
>> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>> ...
>> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
>> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
>> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
>> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
>> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
>> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
>> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
>> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
>> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
>> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
>> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>>
>> Howerver, later patch:
>> commit e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
>> reverts the codes, which causes panic again in arm, so fix it back to the old version.
>>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>> ---
>> fs/f2fs/dir.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index f00b5ed..de2e295 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>> int save_len = fstr->len;
>> int err;
>>
>> + de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
>> + if (!de_name.name)
>> + return -ENOMEM;
>> +
>> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>> +
>> err = fscrypt_fname_disk_to_usr(d->inode,
>> (u32)de->hash_code, 0,
>> &de_name, fstr);
>> + kfree(de_name.name);
>> if (err)
>> return err;
>>
>> --
>> 1.8.5.2
>
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-02-28 9:50 ` Chao Yu
@ 2018-03-01 2:50 ` Jaegeuk Kim
2018-03-01 3:02 ` Chao Yu
0 siblings, 1 reply; 14+ messages in thread
From: Jaegeuk Kim @ 2018-03-01 2:50 UTC (permalink / raw)
To: Chao Yu
Cc: Yunlong Song, chao, yunlong.song, miaoxie, bintian.wang,
shengyong1, heyunlei, linux-f2fs-devel, linux-kernel
On 02/28, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2018/2/28 13:48, Jaegeuk Kim wrote:
> > Hi Yunlong,
> >
> > As Eric pointed out, how do you think using nohighmem for directory likewise
>
> I'd like to ask, at the beginning, why we choose to use highmem for dentry page?
> any history reason there?
There was no huge preference on it based on performance. I just wanted not to
abuse lowmem.
Thanks,
>
> > ext4, which looks like more efficient? Actually, we don't need to do this in
> > most of recent kernels, right?
>
> It's OK to me to keep a line with ext4.
>
> Thanks,
>
> >
> > Thanks,
> >
> > On 02/28, Yunlong Song wrote:
> >> This reverts commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b.
> >>
> >> Conflicts:
> >> fs/f2fs/dir.c
> >>
> >> In some platforms (such as arm), high memory is used, then the
> >> decrypting filename will cause panic, the reason see commit
> >> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
> >> for decrypting filename"):
> >>
> >> We got dentry pages from high_mem, and its address space directly goes into the
> >> decryption path via f2fs_fname_disk_to_usr.
> >> But, sg_init_one assumes the address is not from high_mem, so we can get this
> >> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
> >>
> >> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
> >> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> >> ...
> >> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
> >> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
> >> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
> >> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
> >> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
> >> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
> >> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
> >> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
> >> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
> >> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
> >> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
> >>
> >> Howerver, later patch:
> >> commit e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
> >> reverts the codes, which causes panic again in arm, so fix it back to the old version.
> >>
> >> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> >> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> >> ---
> >> fs/f2fs/dir.c | 7 +++++++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> >> index f00b5ed..de2e295 100644
> >> --- a/fs/f2fs/dir.c
> >> +++ b/fs/f2fs/dir.c
> >> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
> >> int save_len = fstr->len;
> >> int err;
> >>
> >> + de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
> >> + if (!de_name.name)
> >> + return -ENOMEM;
> >> +
> >> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
> >> +
> >> err = fscrypt_fname_disk_to_usr(d->inode,
> >> (u32)de->hash_code, 0,
> >> &de_name, fstr);
> >> + kfree(de_name.name);
> >> if (err)
> >> return err;
> >>
> >> --
> >> 1.8.5.2
> >
> > .
> >
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-03-01 2:50 ` Jaegeuk Kim
@ 2018-03-01 3:02 ` Chao Yu
0 siblings, 0 replies; 14+ messages in thread
From: Chao Yu @ 2018-03-01 3:02 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Yunlong Song, chao, yunlong.song, miaoxie, bintian.wang,
shengyong1, heyunlei, linux-f2fs-devel, linux-kernel
On 2018/3/1 10:50, Jaegeuk Kim wrote:
> On 02/28, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2018/2/28 13:48, Jaegeuk Kim wrote:
>>> Hi Yunlong,
>>>
>>> As Eric pointed out, how do you think using nohighmem for directory likewise
>>
>> I'd like to ask, at the beginning, why we choose to use highmem for dentry page?
>> any history reason there?
>
> There was no huge preference on it based on performance. I just wanted not to
> abuse lowmem.
Got you, thanks for explanation.
Thanks,
>
> Thanks,
>
>>
>>> ext4, which looks like more efficient? Actually, we don't need to do this in
>>> most of recent kernels, right?
>>
>> It's OK to me to keep a line with ext4.
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>> On 02/28, Yunlong Song wrote:
>>>> This reverts commit e06f86e61d7a67fe6e826010f57aa39c674f4b1b.
>>>>
>>>> Conflicts:
>>>> fs/f2fs/dir.c
>>>>
>>>> In some platforms (such as arm), high memory is used, then the
>>>> decrypting filename will cause panic, the reason see commit
>>>> 569cf1876a32e574ba8a7fb825cd91bafd003882 ("f2fs crypto: allocate buffer
>>>> for decrypting filename"):
>>>>
>>>> We got dentry pages from high_mem, and its address space directly goes into the
>>>> decryption path via f2fs_fname_disk_to_usr.
>>>> But, sg_init_one assumes the address is not from high_mem, so we can get this
>>>> panic since it doesn't call kmap_high but kunmap_high is triggered at the end.
>>>>
>>>> kernel BUG at ../../../../../../kernel/mm/highmem.c:290!
>>>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>>>> ...
>>>> (kunmap_high+0xb0/0xb8) from [<c0114534>] (__kunmap_atomic+0xa0/0xa4)
>>>> (__kunmap_atomic+0xa0/0xa4) from [<c035f028>] (blkcipher_walk_done+0x128/0x1ec)
>>>> (blkcipher_walk_done+0x128/0x1ec) from [<c0366c24>] (crypto_cbc_decrypt+0xc0/0x170)
>>>> (crypto_cbc_decrypt+0xc0/0x170) from [<c0367148>] (crypto_cts_decrypt+0xc0/0x114)
>>>> (crypto_cts_decrypt+0xc0/0x114) from [<c035ea98>] (async_decrypt+0x40/0x48)
>>>> (async_decrypt+0x40/0x48) from [<c032ca34>] (f2fs_fname_disk_to_usr+0x124/0x304)
>>>> (f2fs_fname_disk_to_usr+0x124/0x304) from [<c03056fc>] (f2fs_fill_dentries+0xac/0x188)
>>>> (f2fs_fill_dentries+0xac/0x188) from [<c03059c8>] (f2fs_readdir+0x1f0/0x300)
>>>> (f2fs_readdir+0x1f0/0x300) from [<c0218054>] (vfs_readdir+0x90/0xb4)
>>>> (vfs_readdir+0x90/0xb4) from [<c0218418>] (SyS_getdents64+0x64/0xcc)
>>>> (SyS_getdents64+0x64/0xcc) from [<c0105ba0>] (ret_fast_syscall+0x0/0x30)
>>>>
>>>> Howerver, later patch:
>>>> commit e06f86e61d7a ("f2fs crypto: avoid unneeded memory allocation in ->readdir")
>>>> reverts the codes, which causes panic again in arm, so fix it back to the old version.
>>>>
>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>> Reviewed-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>> fs/f2fs/dir.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>>>> index f00b5ed..de2e295 100644
>>>> --- a/fs/f2fs/dir.c
>>>> +++ b/fs/f2fs/dir.c
>>>> @@ -825,9 +825,16 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
>>>> int save_len = fstr->len;
>>>> int err;
>>>>
>>>> + de_name.name = f2fs_kmalloc(sbi, de_name.len, GFP_NOFS);
>>>> + if (!de_name.name)
>>>> + return -ENOMEM;
>>>> +
>>>> + memcpy(de_name.name, d->filename[bit_pos], de_name.len);
>>>> +
>>>> err = fscrypt_fname_disk_to_usr(d->inode,
>>>> (u32)de->hash_code, 0,
>>>> &de_name, fstr);
>>>> + kfree(de_name.name);
>>>> if (err)
>>>> return err;
>>>>
>>>> --
>>>> 1.8.5.2
>>>
>>> .
>>>
>
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] Revert "f2fs crypto: avoid unneeded memory allocation in ->readdir"
2018-02-28 5:48 ` Jaegeuk Kim
2018-02-28 9:50 ` Chao Yu
@ 2018-02-28 12:32 ` Yunlong Song
1 sibling, 0 replies; 14+ messages in thread
From: Yunlong Song @ 2018-02-28 12:32 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: chao, yuchao0, yunlong.song, miaoxie, bintian.wang, shengyong1,
heyunlei, linux-f2fs-devel, linux-kernel
On 2018/2/28 13:48, Jaegeuk Kim wrote:
> Hi Yunlong,
>
> As Eric pointed out, how do you think using nohighmem for directory likewise
> ext4, which looks like more efficient?
OK, I have sent out another patch like this.
Actually, we don't need to do this in
> most of recent kernels, right?
>
Why? I have got this panic using arm with recent kernel.
^ permalink raw reply [flat|nested] 14+ messages in thread