Re: [f2fs-dev] [PATCH 14/18] f2fs crypto: add filename encryption for f2fs_lookup
It's OK now. Thanks, Hu On 2015/5/11 13:12, Jaegeuk Kim wrote: > Hi Hujianynag, > > I just fixed it up, and rebased the dev branch. > Could you check them out? > > Thanks, > > On Mon, May 11, 2015 at 10:52:48AM +0800, hujianyang wrote: >> Hi Jaegeuk, >> >> While compiling >> git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git >> branch dev(commit 5af6e74892a f2fs crypto: remove checking key context >> during lookup), >> I saw an error: >> >> fs/f2fs/dir.c: In function ‘find_in_level’: >> fs/f2fs/dir.c:163: error: unknown field ‘len’ specified in initializer >> fs/f2fs/dir.c:163: warning: excess elements in struct initializer >> fs/f2fs/dir.c:163: warning: (near initialization for ‘name’) >> >> I think it's related to this patch. >> If there is anything wrong in my configuration, please let me know. >> >> Thanks, >> Hu >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [f2fs-dev] [PATCH 14/18] f2fs crypto: add filename encryption for f2fs_lookup
It's OK now. Thanks, Hu On 2015/5/11 13:12, Jaegeuk Kim wrote: Hi Hujianynag, I just fixed it up, and rebased the dev branch. Could you check them out? Thanks, On Mon, May 11, 2015 at 10:52:48AM +0800, hujianyang wrote: Hi Jaegeuk, While compiling git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git branch dev(commit 5af6e74892a f2fs crypto: remove checking key context during lookup), I saw an error: fs/f2fs/dir.c: In function ‘find_in_level’: fs/f2fs/dir.c:163: error: unknown field ‘len’ specified in initializer fs/f2fs/dir.c:163: warning: excess elements in struct initializer fs/f2fs/dir.c:163: warning: (near initialization for ‘name’) I think it's related to this patch. If there is anything wrong in my configuration, please let me know. Thanks, Hu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [f2fs-dev] [PATCH 14/18] f2fs crypto: add filename encryption for f2fs_lookup
Hi Jaegeuk,
While compiling git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git
branch dev(commit 5af6e74892a f2fs crypto: remove checking key context during
lookup),
I saw an error:
fs/f2fs/dir.c: In function ‘find_in_level’:
fs/f2fs/dir.c:163: error: unknown field ‘len’ specified in initializer
fs/f2fs/dir.c:163: warning: excess elements in struct initializer
fs/f2fs/dir.c:163: warning: (near initialization for ‘name’)
I think it's related to this patch.
If there is anything wrong in my configuration, please let me know.
Thanks,
Hu
On 2015/5/9 12:20, Jaegeuk Kim wrote:
> This patch implements filename encryption support for f2fs_lookup.
>
> Note that, f2fs_find_entry should be outside of f2fs_(un)lock_op().
>
> Signed-off-by: Jaegeuk Kim
> ---
> fs/f2fs/dir.c| 79
>
> fs/f2fs/f2fs.h | 9 ---
> fs/f2fs/inline.c | 9 ---
> 3 files changed, 56 insertions(+), 41 deletions(-)
>
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index ab6455d..5e10d9d 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -76,20 +76,10 @@ static unsigned long dir_block_index(unsigned int level,
> return bidx;
> }
>
> -static bool early_match_name(size_t namelen, f2fs_hash_t namehash,
> - struct f2fs_dir_entry *de)
> -{
> - if (le16_to_cpu(de->name_len) != namelen)
> - return false;
> -
> - if (de->hash_code != namehash)
> - return false;
> -
> - return true;
> -}
> -
> static struct f2fs_dir_entry *find_in_block(struct page *dentry_page,
> - struct qstr *name, int *max_slots,
> + struct f2fs_filename *fname,
> + f2fs_hash_t namehash,
> + int *max_slots,
> struct page **res_page)
> {
> struct f2fs_dentry_block *dentry_blk;
> @@ -99,8 +89,7 @@ static struct f2fs_dir_entry *find_in_block(struct page
> *dentry_page,
> dentry_blk = (struct f2fs_dentry_block *)kmap(dentry_page);
>
> make_dentry_ptr(NULL, , (void *)dentry_blk, 1);
> - de = find_target_dentry(name, max_slots, );
> -
> + de = find_target_dentry(fname, namehash, max_slots, );
> if (de)
> *res_page = dentry_page;
> else
> @@ -114,13 +103,15 @@ static struct f2fs_dir_entry *find_in_block(struct page
> *dentry_page,
> return de;
> }
>
> -struct f2fs_dir_entry *find_target_dentry(struct qstr *name, int *max_slots,
> - struct f2fs_dentry_ptr *d)
> +struct f2fs_dir_entry *find_target_dentry(struct f2fs_filename *fname,
> + f2fs_hash_t namehash, int *max_slots,
> + struct f2fs_dentry_ptr *d)
> {
> struct f2fs_dir_entry *de;
> unsigned long bit_pos = 0;
> - f2fs_hash_t namehash = f2fs_dentry_hash(name);
> int max_len = 0;
> + struct f2fs_str de_name = FSTR_INIT(NULL, 0);
> + struct f2fs_str *name = >disk_name;
>
> if (max_slots)
> *max_slots = 0;
> @@ -132,8 +123,18 @@ struct f2fs_dir_entry *find_target_dentry(struct qstr
> *name, int *max_slots,
> }
>
> de = >dentry[bit_pos];
> - if (early_match_name(name->len, namehash, de) &&
> - !memcmp(d->filename[bit_pos], name->name, name->len))
> +
> + /* encrypted case */
> + de_name.name = d->filename[bit_pos];
> + de_name.len = le16_to_cpu(de->name_len);
> +
> + /* show encrypted name */
> + if (fname->hash) {
> + if (de->hash_code == fname->hash)
> + goto found;
> + } else if (de_name.len == name->len &&
> + de->hash_code == namehash &&
> + !memcmp(de_name.name, name->name, name->len))
> goto found;
>
> if (max_slots && max_len > *max_slots)
> @@ -155,16 +156,21 @@ found:
> }
>
> static struct f2fs_dir_entry *find_in_level(struct inode *dir,
> - unsigned int level, struct qstr *name,
> - f2fs_hash_t namehash, struct page **res_page)
> + unsigned int level,
> + struct f2fs_filename *fname,
> + struct page **res_page)
> {
> - int s = GET_DENTRY_SLOTS(name->len);
> + struct qstr name = FSTR_TO_QSTR(>disk_name);
> + int s = GET_DENTRY_SLOTS(name.len);
> unsigned int nbucket, nblock;
> unsigned int bidx, end_block;
> struct page *dentry_page;
> struct f2fs_dir_entry *de = NULL;
> bool room = false;
> int max_slots;
> + f2fs_hash_t namehash;
> +
> + namehash = f2fs_dentry_hash();
>
> f2fs_bug_on(F2FS_I_SB(dir), level > MAX_DIR_HASH_DEPTH);
>
> @@ -183,7 +189,8 @@
Re: [f2fs-dev] [PATCH 14/18] f2fs crypto: add filename encryption for f2fs_lookup
Hi Jaegeuk,
While compiling git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git
branch dev(commit 5af6e74892a f2fs crypto: remove checking key context during
lookup),
I saw an error:
fs/f2fs/dir.c: In function ‘find_in_level’:
fs/f2fs/dir.c:163: error: unknown field ‘len’ specified in initializer
fs/f2fs/dir.c:163: warning: excess elements in struct initializer
fs/f2fs/dir.c:163: warning: (near initialization for ‘name’)
I think it's related to this patch.
If there is anything wrong in my configuration, please let me know.
Thanks,
Hu
On 2015/5/9 12:20, Jaegeuk Kim wrote:
This patch implements filename encryption support for f2fs_lookup.
Note that, f2fs_find_entry should be outside of f2fs_(un)lock_op().
Signed-off-by: Jaegeuk Kim jaeg...@kernel.org
---
fs/f2fs/dir.c| 79
fs/f2fs/f2fs.h | 9 ---
fs/f2fs/inline.c | 9 ---
3 files changed, 56 insertions(+), 41 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index ab6455d..5e10d9d 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -76,20 +76,10 @@ static unsigned long dir_block_index(unsigned int level,
return bidx;
}
-static bool early_match_name(size_t namelen, f2fs_hash_t namehash,
- struct f2fs_dir_entry *de)
-{
- if (le16_to_cpu(de-name_len) != namelen)
- return false;
-
- if (de-hash_code != namehash)
- return false;
-
- return true;
-}
-
static struct f2fs_dir_entry *find_in_block(struct page *dentry_page,
- struct qstr *name, int *max_slots,
+ struct f2fs_filename *fname,
+ f2fs_hash_t namehash,
+ int *max_slots,
struct page **res_page)
{
struct f2fs_dentry_block *dentry_blk;
@@ -99,8 +89,7 @@ static struct f2fs_dir_entry *find_in_block(struct page
*dentry_page,
dentry_blk = (struct f2fs_dentry_block *)kmap(dentry_page);
make_dentry_ptr(NULL, d, (void *)dentry_blk, 1);
- de = find_target_dentry(name, max_slots, d);
-
+ de = find_target_dentry(fname, namehash, max_slots, d);
if (de)
*res_page = dentry_page;
else
@@ -114,13 +103,15 @@ static struct f2fs_dir_entry *find_in_block(struct page
*dentry_page,
return de;
}
-struct f2fs_dir_entry *find_target_dentry(struct qstr *name, int *max_slots,
- struct f2fs_dentry_ptr *d)
+struct f2fs_dir_entry *find_target_dentry(struct f2fs_filename *fname,
+ f2fs_hash_t namehash, int *max_slots,
+ struct f2fs_dentry_ptr *d)
{
struct f2fs_dir_entry *de;
unsigned long bit_pos = 0;
- f2fs_hash_t namehash = f2fs_dentry_hash(name);
int max_len = 0;
+ struct f2fs_str de_name = FSTR_INIT(NULL, 0);
+ struct f2fs_str *name = fname-disk_name;
if (max_slots)
*max_slots = 0;
@@ -132,8 +123,18 @@ struct f2fs_dir_entry *find_target_dentry(struct qstr
*name, int *max_slots,
}
de = d-dentry[bit_pos];
- if (early_match_name(name-len, namehash, de)
- !memcmp(d-filename[bit_pos], name-name, name-len))
+
+ /* encrypted case */
+ de_name.name = d-filename[bit_pos];
+ de_name.len = le16_to_cpu(de-name_len);
+
+ /* show encrypted name */
+ if (fname-hash) {
+ if (de-hash_code == fname-hash)
+ goto found;
+ } else if (de_name.len == name-len
+ de-hash_code == namehash
+ !memcmp(de_name.name, name-name, name-len))
goto found;
if (max_slots max_len *max_slots)
@@ -155,16 +156,21 @@ found:
}
static struct f2fs_dir_entry *find_in_level(struct inode *dir,
- unsigned int level, struct qstr *name,
- f2fs_hash_t namehash, struct page **res_page)
+ unsigned int level,
+ struct f2fs_filename *fname,
+ struct page **res_page)
{
- int s = GET_DENTRY_SLOTS(name-len);
+ struct qstr name = FSTR_TO_QSTR(fname-disk_name);
+ int s = GET_DENTRY_SLOTS(name.len);
unsigned int nbucket, nblock;
unsigned int bidx, end_block;
struct page *dentry_page;
struct f2fs_dir_entry *de = NULL;
bool room = false;
int max_slots;
+ f2fs_hash_t namehash;
+
+ namehash = f2fs_dentry_hash(name);
f2fs_bug_on(F2FS_I_SB(dir), level MAX_DIR_HASH_DEPTH);
@@ -183,7 +189,8 @@ static struct f2fs_dir_entry *find_in_level(struct inode
*dir,
continue;
Re: Patch Issues
Hi Nick,
I'm not quite sure about if it is a correct modification. But,
On 2015/1/16 10:18, nick wrote:
> drivers/mtd/inftlmount.c:336:12: warning: ‘check_free_sectors’ defined but
> not used [-Wunused-function]
check if this function is still called by other functions, if it
is not, just remove it in your patch.
> static int check_free_sectors(struct INFTLrecord *inftl, unsigned int
> address,
> ^
> drivers/mtd/inftlmount.c: In function ‘INFTL_formatblock’:
> drivers/mtd/inftlmount.c:781:1: warning: control reaches end of non-void
> function [-Wreturn-type]
> }
> Patch:
> From 6b481c8f5030da2e9616bd038193d68340c0b5d0 Mon Sep 17 00:00:00 2001
> 2 From: Nicholas Krause
> 3 Date: Thu, 15 Jan 2015 20:10:37 -0500
> 4 Subject: [PATCH] mtd: Remove unneeded call to check_free_sectors in the
> 5 function,INFTL_formatblock
> 6
> 7 Removes unneeded call to check_free_sectors internally in the
> function,INFTL_formatblock.
> 8 This call is no longer needed due to us checking to see if erasing the
> block against the
> 9 structure pointer passed to the function,inftl internal variable state is
> equal to the
> 10 macro,MTD_ERASE_FAILED to see if the block has failed in being erased
> successfully.Due
> 11 to this we can remove the no longer needed check to check_free_sectors
> and comments
> 12 related to questioning the reason for it's use with the check against
> MTD_ERASE_FAILED
> 13 for inftl's state variable already checking for successfully erasing of
> the mtd block.
> 14
> 15 Signed-off-by: Nicholas Krause
> 16 ---
> 17 drivers/mtd/inftlmount.c | 10 --
> 18 1 file changed, 10 deletions(-)
> 19
> 20 diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
> 21 index 1388c8d..def5cea 100644
> 22 --- a/drivers/mtd/inftlmount.c
> 23 +++ b/drivers/mtd/inftlmount.c
> 24 @@ -367,7 +367,6 @@ static int check_free_sectors(struct INFTLrecord
> *inftl, unsigned int address,
> 25 *
> 26 * Return: 0 when succeed, -1 on error.
> 27 *
> 28 - * ToDo: 1. Is it necessary to check_free_sector after erasing ??
> 29 */
> 30 int INFTL_formatblock(struct INFTLrecord *inftl, int block)
> 31 {
> 32 @@ -401,15 +400,6 @@ int INFTL_formatblock(struct INFTLrecord *inftl, int
> block)
> 33 goto fail;
> 34 }
> 35
> 36 - /*
> 37 -* Check the "freeness" of Erase Unit before updating
> metadata.
> 38 -* FixMe: is this check really necessary? Since we have
> check
> 39 -* the return code after the erase operation.
> 40 -*/
> 41 - if (check_free_sectors(inftl, instr->addr, instr->len, 1)
> != 0)
> 42 - goto fail;
> 43 - }
You should keep this '}'.
> 44 -
> 45 uci.EraseMark = cpu_to_le16(ERASE_MARK);
> 46 uci.EraseMark1 = cpu_to_le16(ERASE_MARK);
> 47 uci.Reserved[0] = 0;
> 48 --
> 49 2.1.0
> 50
>
> __
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Patch Issues
Hi Nick,
I'm not quite sure about if it is a correct modification. But,
On 2015/1/16 10:18, nick wrote:
drivers/mtd/inftlmount.c:336:12: warning: ‘check_free_sectors’ defined but
not used [-Wunused-function]
check if this function is still called by other functions, if it
is not, just remove it in your patch.
static int check_free_sectors(struct INFTLrecord *inftl, unsigned int
address,
^
drivers/mtd/inftlmount.c: In function ‘INFTL_formatblock’:
drivers/mtd/inftlmount.c:781:1: warning: control reaches end of non-void
function [-Wreturn-type]
}
Patch:
From 6b481c8f5030da2e9616bd038193d68340c0b5d0 Mon Sep 17 00:00:00 2001
2 From: Nicholas Krause xerofo...@gmail.com
3 Date: Thu, 15 Jan 2015 20:10:37 -0500
4 Subject: [PATCH] mtd: Remove unneeded call to check_free_sectors in the
5 function,INFTL_formatblock
6
7 Removes unneeded call to check_free_sectors internally in the
function,INFTL_formatblock.
8 This call is no longer needed due to us checking to see if erasing the
block against the
9 structure pointer passed to the function,inftl internal variable state is
equal to the
10 macro,MTD_ERASE_FAILED to see if the block has failed in being erased
successfully.Due
11 to this we can remove the no longer needed check to check_free_sectors
and comments
12 related to questioning the reason for it's use with the check against
MTD_ERASE_FAILED
13 for inftl's state variable already checking for successfully erasing of
the mtd block.
14
15 Signed-off-by: Nicholas Krause xerofo...@gmail.com
16 ---
17 drivers/mtd/inftlmount.c | 10 --
18 1 file changed, 10 deletions(-)
19
20 diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
21 index 1388c8d..def5cea 100644
22 --- a/drivers/mtd/inftlmount.c
23 +++ b/drivers/mtd/inftlmount.c
24 @@ -367,7 +367,6 @@ static int check_free_sectors(struct INFTLrecord
*inftl, unsigned int address,
25 *
26 * Return: 0 when succeed, -1 on error.
27 *
28 - * ToDo: 1. Is it necessary to check_free_sector after erasing ??
29 */
30 int INFTL_formatblock(struct INFTLrecord *inftl, int block)
31 {
32 @@ -401,15 +400,6 @@ int INFTL_formatblock(struct INFTLrecord *inftl, int
block)
33 goto fail;
34 }
35
36 - /*
37 -* Check the freeness of Erase Unit before updating
metadata.
38 -* FixMe: is this check really necessary? Since we have
check
39 -* the return code after the erase operation.
40 -*/
41 - if (check_free_sectors(inftl, instr-addr, instr-len, 1)
!= 0)
42 - goto fail;
43 - }
You should keep this '}'.
44 -
45 uci.EraseMark = cpu_to_le16(ERASE_MARK);
46 uci.EraseMark1 = cpu_to_le16(ERASE_MARK);
47 uci.Reserved[0] = 0;
48 --
49 2.1.0
50
__
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] blk-mq: cleanup additional nr_hw_queues check
No need to check set->nr_hw_queues twice in blk_mq_alloc_tag_set(),
remove the latter one.
Signed-off-by: hujianyang
---
block/blk-mq.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a7d4a98..eddeccc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2097,7 +2097,7 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
if (set->queue_depth < set->reserved_tags + BLK_MQ_TAG_MIN)
return -EINVAL;
- if (!set->nr_hw_queues || !set->ops->queue_rq || !set->ops->map_queue)
+ if (!set->ops->queue_rq || !set->ops->map_queue)
return -EINVAL;
if (set->queue_depth > BLK_MQ_MAX_DEPTH) {
--
1.6.0.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] blk-mq: cleanup additional nr_hw_queues check
No need to check set-nr_hw_queues twice in blk_mq_alloc_tag_set(),
remove the latter one.
Signed-off-by: hujianyang hujiany...@huawei.com
---
block/blk-mq.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a7d4a98..eddeccc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2097,7 +2097,7 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
if (set-queue_depth set-reserved_tags + BLK_MQ_TAG_MIN)
return -EINVAL;
- if (!set-nr_hw_queues || !set-ops-queue_rq || !set-ops-map_queue)
+ if (!set-ops-queue_rq || !set-ops-map_queue)
return -EINVAL;
if (set-queue_depth BLK_MQ_MAX_DEPTH) {
--
1.6.0.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Fix condition check for workdir
On 2015/1/8 20:41, Seunghun Lee wrote:
> When file system is mounted read-only workdir is not needed.
>
> Signed-off-by: Seunghun Lee
> ---
> fs/overlayfs/super.c | 35 +++
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 84f3144..4e50617 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -30,6 +30,7 @@ struct ovl_config {
> char *lowerdir;
> char *upperdir;
> char *workdir;
> + bool is_rw;
> };
>
> /* private information held for overlayfs's superblock */
> @@ -515,10 +516,11 @@ static int ovl_show_options(struct seq_file *m, struct
> dentry *dentry)
> struct ovl_fs *ufs = sb->s_fs_info;
>
> seq_printf(m, ",lowerdir=%s", ufs->config.lowerdir);
> - if (ufs->config.upperdir) {
> + if (ufs->config.upperdir)
> seq_printf(m, ",upperdir=%s", ufs->config.upperdir);
> + if (ufs->config.is_rw)
> seq_printf(m, ",workdir=%s", ufs->config.workdir);
> - }
> +
> return 0;
> }
>
> @@ -822,16 +824,27 @@ static int ovl_fill_super(struct super_block *sb, void
> *data, int silent)
>
> sb->s_stack_depth = 0;
> if (ufs->config.upperdir) {
> - /* FIXME: workdir is not needed for a R/O mount */
> + err = ovl_mount_dir(ufs->config.upperdir, );
> + if (err)
> + goto out_free_config;
> +
> + sb->s_stack_depth = upperpath.mnt->mnt_sb->s_stack_depth;
> + }
> +
> + /* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too */
> + if (!upperpath.mnt || (upperpath.mnt->mnt_sb->s_flags & MS_RDONLY)) {
> + ufs->config.is_rw = false;
> + sb->s_flags |= MS_RDONLY;
> + } else {
> + ufs->config.is_rw = true;
> + }
Hi Seunghun,
A partition can be mounted with ro flag. I think workdir is not
needed in this way either.
> +
> + if (ufs->config.is_rw) {
> if (!ufs->config.workdir) {
> pr_err("overlayfs: missing 'workdir'\n");
> goto out_free_config;
> }
>
> - err = ovl_mount_dir(ufs->config.upperdir, );
> - if (err)
> - goto out_free_config;
> -
> err = ovl_mount_dir(ufs->config.workdir, );
> if (err)
> goto out_put_upperpath;
> @@ -844,8 +857,8 @@ static int ovl_fill_super(struct super_block *sb, void
> *data, int silent)
> pr_err("overlayfs: workdir and upperdir must be
> separate subtrees\n");
> goto out_put_workpath;
> }
> - sb->s_stack_depth = upperpath.mnt->mnt_sb->s_stack_depth;
> }
> +
> err = -ENOMEM;
> lowertmp = kstrdup(ufs->config.lowerdir, GFP_KERNEL);
> if (!lowertmp)
> @@ -884,7 +897,9 @@ static int ovl_fill_super(struct super_block *sb, void
> *data, int silent)
> pr_err("overlayfs: failed to clone upperpath\n");
> goto out_put_lowerpath;
> }
> + }
>
> + if (ufs->config.is_rw) {
> ufs->workdir = ovl_workdir_create(ufs->upper_mnt,
> workpath.dentry);
> err = PTR_ERR(ufs->workdir);
> if (IS_ERR(ufs->workdir)) {
> @@ -914,10 +929,6 @@ static int ovl_fill_super(struct super_block *sb, void
> *data, int silent)
> ufs->numlower++;
> }
>
> - /* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too */
> - if (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY))
> - sb->s_flags |= MS_RDONLY;
> -
> sb->s_d_op = _dentry_operations;
>
> err = -ENOMEM;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Fix condition check for workdir
On 2015/1/8 20:41, Seunghun Lee wrote:
When file system is mounted read-only workdir is not needed.
Signed-off-by: Seunghun Lee way...@gmail.com
---
fs/overlayfs/super.c | 35 +++
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 84f3144..4e50617 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -30,6 +30,7 @@ struct ovl_config {
char *lowerdir;
char *upperdir;
char *workdir;
+ bool is_rw;
};
/* private information held for overlayfs's superblock */
@@ -515,10 +516,11 @@ static int ovl_show_options(struct seq_file *m, struct
dentry *dentry)
struct ovl_fs *ufs = sb-s_fs_info;
seq_printf(m, ,lowerdir=%s, ufs-config.lowerdir);
- if (ufs-config.upperdir) {
+ if (ufs-config.upperdir)
seq_printf(m, ,upperdir=%s, ufs-config.upperdir);
+ if (ufs-config.is_rw)
seq_printf(m, ,workdir=%s, ufs-config.workdir);
- }
+
return 0;
}
@@ -822,16 +824,27 @@ static int ovl_fill_super(struct super_block *sb, void
*data, int silent)
sb-s_stack_depth = 0;
if (ufs-config.upperdir) {
- /* FIXME: workdir is not needed for a R/O mount */
+ err = ovl_mount_dir(ufs-config.upperdir, upperpath);
+ if (err)
+ goto out_free_config;
+
+ sb-s_stack_depth = upperpath.mnt-mnt_sb-s_stack_depth;
+ }
+
+ /* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too */
+ if (!upperpath.mnt || (upperpath.mnt-mnt_sb-s_flags MS_RDONLY)) {
+ ufs-config.is_rw = false;
+ sb-s_flags |= MS_RDONLY;
+ } else {
+ ufs-config.is_rw = true;
+ }
Hi Seunghun,
A partition can be mounted with ro flag. I think workdir is not
needed in this way either.
+
+ if (ufs-config.is_rw) {
if (!ufs-config.workdir) {
pr_err(overlayfs: missing 'workdir'\n);
goto out_free_config;
}
- err = ovl_mount_dir(ufs-config.upperdir, upperpath);
- if (err)
- goto out_free_config;
-
err = ovl_mount_dir(ufs-config.workdir, workpath);
if (err)
goto out_put_upperpath;
@@ -844,8 +857,8 @@ static int ovl_fill_super(struct super_block *sb, void
*data, int silent)
pr_err(overlayfs: workdir and upperdir must be
separate subtrees\n);
goto out_put_workpath;
}
- sb-s_stack_depth = upperpath.mnt-mnt_sb-s_stack_depth;
}
+
err = -ENOMEM;
lowertmp = kstrdup(ufs-config.lowerdir, GFP_KERNEL);
if (!lowertmp)
@@ -884,7 +897,9 @@ static int ovl_fill_super(struct super_block *sb, void
*data, int silent)
pr_err(overlayfs: failed to clone upperpath\n);
goto out_put_lowerpath;
}
+ }
+ if (ufs-config.is_rw) {
ufs-workdir = ovl_workdir_create(ufs-upper_mnt,
workpath.dentry);
err = PTR_ERR(ufs-workdir);
if (IS_ERR(ufs-workdir)) {
@@ -914,10 +929,6 @@ static int ovl_fill_super(struct super_block *sb, void
*data, int silent)
ufs-numlower++;
}
- /* If the upper fs is r/o or nonexistent, we mark overlayfs r/o too */
- if (!ufs-upper_mnt || (ufs-upper_mnt-mnt_sb-s_flags MS_RDONLY))
- sb-s_flags |= MS_RDONLY;
-
sb-s_d_op = ovl_dentry_operations;
err = -ENOMEM;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Prevent rw remount when it should be ro mount
Hi, There maybe some misunderstandings here. I think your patch really fix an important problem, but not in correct way. On 2015/1/6 22:02, Seunghun Lee wrote: > > After patch: > root@qemux86:~# mount -t overlay overlay -olowerdir=lower:lower2 merged > mount: warning: merged seems to be mounted read-only. > root@qemux86:~# mount | grep overlay > overlay on /home/root/merged type overlay (ro,relatime,lowerdir=lower:lower2) > root@qemux86:~# mount -o remount,rw merged > mount: warning: /home/root/merged seems to be mounted read-only. > root@qemux86:~# mount | grep overlay > overlay on /home/root/merged type overlay (ro,relatime,lowerdir=lower:lower2) > root@qemux86:~# echo hi > merged/hi > -sh: merged/hi: Read-only file system > root@qemux86:~# > If users want a rw mount, can we give them a ro mount? I think it's wrong, .remount_fs should refuse this request. So I think your .remount_fs should check both what users in userpace want and what kernel can offer, then realize legal requests and refuse illegal requests. Not changing the requests from users. Further more, can we replace upper/lower/work directories or mount point by this .remount_fs? If you want to export a new function, I think you should considering more about these. Thanks, Hu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Prevent rw remount when it should be ro mount
Hi, There maybe some misunderstandings here. I think your patch really fix an important problem, but not in correct way. On 2015/1/6 22:02, Seunghun Lee wrote: After patch: root@qemux86:~# mount -t overlay overlay -olowerdir=lower:lower2 merged mount: warning: merged seems to be mounted read-only. root@qemux86:~# mount | grep overlay overlay on /home/root/merged type overlay (ro,relatime,lowerdir=lower:lower2) root@qemux86:~# mount -o remount,rw merged mount: warning: /home/root/merged seems to be mounted read-only. root@qemux86:~# mount | grep overlay overlay on /home/root/merged type overlay (ro,relatime,lowerdir=lower:lower2) root@qemux86:~# echo hi merged/hi -sh: merged/hi: Read-only file system root@qemux86:~# If users want a rw mount, can we give them a ro mount? I think it's wrong, .remount_fs should refuse this request. So I think your .remount_fs should check both what users in userpace want and what kernel can offer, then realize legal requests and refuse illegal requests. Not changing the requests from users. Further more, can we replace upper/lower/work directories or mount point by this .remount_fs? If you want to export a new function, I think you should considering more about these. Thanks, Hu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Prevent rw remount when it should be ro mount
On 2015/1/3 1:26, Seunghun Lee wrote:
> Overlayfs should be mounted read-only when upper-fs is read-only or
> nonexistent.
> But now it can be remounted read-write and this can cause kernel panic.
> So we should prevent read-write remount when the above situation happens.
>
> Signed-off-by: Seunghun Lee
> ---
> fs/overlayfs/super.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 84f3144..8944651 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -522,10 +522,21 @@ static int ovl_show_options(struct seq_file *m, struct
> dentry *dentry)
> return 0;
> }
>
> +static int ovl_remount(struct super_block *sb, int *flags, char *data)
> +{
> + struct ovl_fs *ufs = sb->s_fs_info;
> +
> + if (!ufs->upper_mnt || (ufs->upper_mnt->mnt_sb->s_flags & MS_RDONLY))
> + *flags |= MS_RDONLY;
> +
> + return 0;
> +}
> +
> static const struct super_operations ovl_super_operations = {
> .put_super = ovl_put_super,
> .statfs = ovl_statfs,
> .show_options = ovl_show_options,
> + .remount_fs = ovl_remount,
> };
>
> enum {
>
I think this exporting of .remount_fs may allow people in userspace
have the ability to remount a filesystem with a new set of mounting
options. Your new adding function do nothing with the passing in
parameters.
I'm not sure if it could be competent for remount case.
Add Cc linux-unionfs.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ovl: Prevent rw remount when it should be ro mount
On 2015/1/3 1:26, Seunghun Lee wrote:
Overlayfs should be mounted read-only when upper-fs is read-only or
nonexistent.
But now it can be remounted read-write and this can cause kernel panic.
So we should prevent read-write remount when the above situation happens.
Signed-off-by: Seunghun Lee way...@gmail.com
---
fs/overlayfs/super.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 84f3144..8944651 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -522,10 +522,21 @@ static int ovl_show_options(struct seq_file *m, struct
dentry *dentry)
return 0;
}
+static int ovl_remount(struct super_block *sb, int *flags, char *data)
+{
+ struct ovl_fs *ufs = sb-s_fs_info;
+
+ if (!ufs-upper_mnt || (ufs-upper_mnt-mnt_sb-s_flags MS_RDONLY))
+ *flags |= MS_RDONLY;
+
+ return 0;
+}
+
static const struct super_operations ovl_super_operations = {
.put_super = ovl_put_super,
.statfs = ovl_statfs,
.show_options = ovl_show_options,
+ .remount_fs = ovl_remount,
};
enum {
I think this exporting of .remount_fs may allow people in userspace
have the ability to remount a filesystem with a new set of mounting
options. Your new adding function do nothing with the passing in
parameters.
I'm not sure if it could be competent for remount case.
Add Cc linux-unionfs.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/12/29 22:15, Tanya Brokhman wrote: > Hi Hu, > > On 12/29/2014 5:14 AM, hujianyang wrote: >> On 2014/11/3 21:58, Tanya Brokhman wrote: >>> If there is more then one UBI device mounted, there is no way to >>> distinguish between messages from different UBI devices. >>> Add device number to all ubi layer message types. >>> >>> The R/O block driver messages were replaced by pr_* since >>> ubi_device structure is not used by it. >>> >>> Amended a bit by Artem. >>> >>> Signed-off-by: Tanya Brokhman >>> Signed-off-by: Artem Bityutskiy >>> --- >>> Changes from V1: >> >> >> Hi Artem and Tanya, >> >> Do you think the similar effort is worth to be done on ubifs-level? > > We already did this for UBIFS as well and have been working with this patch > for some time now. Its in our todo to share it, just got pulled into some > urgent staff unfortunately. > Got it~! I'd like to wait for your patch, thanks for your work. Hu > > Thanks, > Tanya Brokhman -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/12/29 22:15, Tanya Brokhman wrote: Hi Hu, On 12/29/2014 5:14 AM, hujianyang wrote: On 2014/11/3 21:58, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com --- Changes from V1: Hi Artem and Tanya, Do you think the similar effort is worth to be done on ubifs-level? We already did this for UBIFS as well and have been working with this patch for some time now. Its in our todo to share it, just got pulled into some urgent staff unfortunately. Got it~! I'd like to wait for your patch, thanks for your work. Hu Thanks, Tanya Brokhman -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/11/3 21:58, Tanya Brokhman wrote: > If there is more then one UBI device mounted, there is no way to > distinguish between messages from different UBI devices. > Add device number to all ubi layer message types. > > The R/O block driver messages were replaced by pr_* since > ubi_device structure is not used by it. > > Amended a bit by Artem. > > Signed-off-by: Tanya Brokhman > Signed-off-by: Artem Bityutskiy > --- > Changes from V1: Hi Artem and Tanya, Do you think the similar effort is worth to be done on ubifs-level? We could have several ubifs partition mounted on a host. Thanks, Hu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/11/3 21:58, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com --- Changes from V1: Hi Artem and Tanya, Do you think the similar effort is worth to be done on ubifs-level? We could have several ubifs partition mounted on a host. Thanks, Hu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/11/5 23:35, Artem Bityutskiy wrote: > On Mon, 2014-11-03 at 15:58 +0200, Tanya Brokhman wrote: >> If there is more then one UBI device mounted, there is no way to >> distinguish between messages from different UBI devices. >> Add device number to all ubi layer message types. >> >> The R/O block driver messages were replaced by pr_* since >> ubi_device structure is not used by it. >> >> Amended a bit by Artem. >> >> Signed-off-by: Tanya Brokhman >> Signed-off-by: Artem Bityutskiy > > Oh, the entire patch-set again... this means we need to review it again! > Please, have a mercy! :-) It is huge! Please, send a small patch against > the one which I pushed already. Thanks! > That means I can't add a Reviewed-by on this huge one... What a pity~! ^.^ > Artem. > > > __ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6] UBI: Extend UBI layer debug/messaging capabilities
On 2014/11/5 23:35, Artem Bityutskiy wrote: On Mon, 2014-11-03 at 15:58 +0200, Tanya Brokhman wrote: If there is more then one UBI device mounted, there is no way to distinguish between messages from different UBI devices. Add device number to all ubi layer message types. The R/O block driver messages were replaced by pr_* since ubi_device structure is not used by it. Amended a bit by Artem. Signed-off-by: Tanya Brokhman tlin...@codeaurora.org Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com Oh, the entire patch-set again... this means we need to review it again! Please, have a mercy! :-) It is huge! Please, send a small patch against the one which I pushed already. Thanks! That means I can't add a Reviewed-by on this huge one... What a pity~! ^.^ Artem. __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
Hi Tanya,
On 2014/11/3 1:14, Tanya Brokhman wrote:
>>
>> This patch add 'struct ubi_device *' for 3 functions. We can get
>> 'ubi_device' from
>> 'ubi_volume'. So I think it's because when we call these functions, the
>> '->ubi'
>> pointer of 'ubi_volume' is not initialized, am I right? This patch use
>> 'vol->ubi'
>> to indicate a 'struct ubi_device *' pointer in some places, I think you are
>> sure
>> of using them.
>>
>
> 1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of
> the attach process so we need struct ubi_device
> 2. for get_exclusive() - you're right. Will fetch dev number from the volume
> 3. for check_av() - you're right. fixed
>
I'm not sure if 'ubi_volume->ubi' is initialized when we call some kinds of
ubi_err() to print error messages. The reference to a null pointer, we perform
'ubi->ubi_num' in ubi_err(), may crash the kernel. So you should be careful
of these situations not only in above cases but also in other places in your
patch.
>>
>> We have the parameter 'ubi_num' for log in some functions like
>> 'ubi_attach_mtd_dev'
>> before. This patch remove 'ubi_num' in upper changes but keep it in other
>> changes.
>> Do we have a discussed rule to deal with this situation? It's not a big
>> problem~
>
> I removed it because it made no sense printing it twice:
> "ubi-0: attached mtd-0 (...) to ubi0"?
> so I shortned the message:
> "ubi-0: attched mtd..."
> All the info is still there
> Same for other messages that printed ubi number.
>
Could we remove some the 'ubi_num'? I think there are no need to print it
twice in other places, like:
@@ -921,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err("ubi%d already exists", ubi_num);
+ ubi_err(ubi, "ubi%d already exists", ubi_num);
return -EEXIST;
}
}
and
@@ -970,7 +974,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(>fm_mutex);
init_rwsem(>fm_sem);
- ubi_msg("attaching mtd%d to ubi%d", mtd->index, ubi_num);
+ ubi_msg(ubi, "attaching mtd%d to ubi%d", mtd->index, ubi_num);
>>> @@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
>>> pnum, int offset, int len)
>>> return 0;
>>>
>>> fail:
>>> -ubi_err("self-check failed for PEB %d", pnum);
>>> -ubi_msg("hex dump of the %d-%d region", offset, offset + len);
>>> +ubi_err(ubi, "self-check failed for PEB %d", pnum);
>>> +ubi_msg(ubi, "hex dump of the %d-%d region",
>>> + offset, offset + len);
>>> print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len,
>>> 1);
>>> err = -EINVAL;
>>> error:
>>
>> Artem, I know you have tried to align the message code in different lines,
>> maybe
>> you can check if you lose this one.
>>
>
> hmmm... not sure I understand what is wrong here
>
Turn
+ubi_msg(ubi, "hex dump of the %d-%d region",
+ offset, offset + len);
to
+ubi_msg(ubi, "hex dump of the %d-%d region",
+offset, offset + len);
Maybe like this. The next line aligns to the message in first line, not a big
problem.
By the way, I use space in this example, it's wrong. Tab is right.
Thanks!
Hu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
Hi Tanya,
On 2014/11/3 1:14, Tanya Brokhman wrote:
This patch add 'struct ubi_device *' for 3 functions. We can get
'ubi_device' from
'ubi_volume'. So I think it's because when we call these functions, the
'-ubi'
pointer of 'ubi_volume' is not initialized, am I right? This patch use
'vol-ubi'
to indicate a 'struct ubi_device *' pointer in some places, I think you are
sure
of using them.
1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of
the attach process so we need struct ubi_device
2. for get_exclusive() - you're right. Will fetch dev number from the volume
3. for check_av() - you're right. fixed
I'm not sure if 'ubi_volume-ubi' is initialized when we call some kinds of
ubi_err() to print error messages. The reference to a null pointer, we perform
'ubi-ubi_num' in ubi_err(), may crash the kernel. So you should be careful
of these situations not only in above cases but also in other places in your
patch.
We have the parameter 'ubi_num' for log in some functions like
'ubi_attach_mtd_dev'
before. This patch remove 'ubi_num' in upper changes but keep it in other
changes.
Do we have a discussed rule to deal with this situation? It's not a big
problem~
I removed it because it made no sense printing it twice:
ubi-0: attached mtd-0 (...) to ubi0?
so I shortned the message:
ubi-0: attched mtd...
All the info is still there
Same for other messages that printed ubi number.
Could we remove some the 'ubi_num'? I think there are no need to print it
twice in other places, like:
@@ -921,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err(ubi%d already exists, ubi_num);
+ ubi_err(ubi, ubi%d already exists, ubi_num);
return -EEXIST;
}
}
and
@@ -970,7 +974,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(ubi-fm_mutex);
init_rwsem(ubi-fm_sem);
- ubi_msg(attaching mtd%d to ubi%d, mtd-index, ubi_num);
+ ubi_msg(ubi, attaching mtd%d to ubi%d, mtd-index, ubi_num);
@@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int
pnum, int offset, int len)
return 0;
fail:
-ubi_err(self-check failed for PEB %d, pnum);
-ubi_msg(hex dump of the %d-%d region, offset, offset + len);
+ubi_err(ubi, self-check failed for PEB %d, pnum);
+ubi_msg(ubi, hex dump of the %d-%d region,
+ offset, offset + len);
print_hex_dump(KERN_DEBUG, , DUMP_PREFIX_OFFSET, 32, 1, buf, len,
1);
err = -EINVAL;
error:
Artem, I know you have tried to align the message code in different lines,
maybe
you can check if you lose this one.
hmmm... not sure I understand what is wrong here
Turn
+ubi_msg(ubi, hex dump of the %d-%d region,
+ offset, offset + len);
to
+ubi_msg(ubi, hex dump of the %d-%d region,
+offset, offset + len);
Maybe like this. The next line aligns to the message in first line, not a big
problem.
By the way, I use space in this example, it's wrong. Tab is right.
Thanks!
Hu
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()
On 2014/10/31 16:09, Richard Weinberger wrote: > Hujianyang, > > Am 31.10.2014 um 05:03 schrieb hujianyang: >> Hi Artem and Richard, >> >> We are using atomic operation, leb_change(), for master_node >> in ubifs-level. We use two lebs for master_node even if they >> are changed with atomic operation. >> >> I think volume_table and master_node play similar roles. Do >> you think changing VTBL record into one peb is OK? I just >> what to know if I missed something. Could you please take >> some time to explain that? > > I'm not sure if I correctly understand your question. > > If we use only one PEB for the VTBL existing UBI implementations > would break as they assume we have two. > > Thanks, > //richard > > __ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > This question is basing on your comment for this patch: """ we can guarantee that the first VTBL record is always correct and we don't really need the second one anymore. """ I think that means one PEB is enough in your considering. So I want to know if you are sure about this. Because we use two leb for master_node in ubifs-level. So maybe VTBL is like super_node, not master_node, right? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()
On 2014/10/31 16:09, Richard Weinberger wrote: Hujianyang, Am 31.10.2014 um 05:03 schrieb hujianyang: Hi Artem and Richard, We are using atomic operation, leb_change(), for master_node in ubifs-level. We use two lebs for master_node even if they are changed with atomic operation. I think volume_table and master_node play similar roles. Do you think changing VTBL record into one peb is OK? I just what to know if I missed something. Could you please take some time to explain that? I'm not sure if I correctly understand your question. If we use only one PEB for the VTBL existing UBI implementations would break as they assume we have two. Thanks, //richard __ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ This question is basing on your comment for this patch: we can guarantee that the first VTBL record is always correct and we don't really need the second one anymore. I think that means one PEB is enough in your considering. So I want to know if you are sure about this. Because we use two leb for master_node in ubifs-level. So maybe VTBL is like super_node, not master_node, right? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()
On 2014/10/30 16:55, Artem Bityutskiy wrote: > On Sat, 2014-10-25 at 19:43 +0200, Richard Weinberger wrote: >> This is more a cosmetic change than a fix. >> By using ubi_eba_atomic_leb_change() >> we can guarantee that the first VTBL record is always >> correct and we don't really need the second one anymore. >> But we have to keep the second one to not break anything. >> >> Signed-off-by: Richard Weinberger > > Yeah, this atomic change stuff was added later, and we had not > envisioned originally. Your patch adds robustness, but makes volume > creation slower, which is probably not a problem. > > I've added a small comment and pushed it, thanks! > > Artem > > Hi Artem and Richard, We are using atomic operation, leb_change(), for master_node in ubifs-level. We use two lebs for master_node even if they are changed with atomic operation. I think volume_table and master_node play similar roles. Do you think changing VTBL record into one peb is OK? I just what to know if I missed something. Could you please take some time to explain that? Thanks very much~! Hu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] UBI: vtbl: Use ubi_eba_atomic_leb_change()
On 2014/10/30 16:55, Artem Bityutskiy wrote: On Sat, 2014-10-25 at 19:43 +0200, Richard Weinberger wrote: This is more a cosmetic change than a fix. By using ubi_eba_atomic_leb_change() we can guarantee that the first VTBL record is always correct and we don't really need the second one anymore. But we have to keep the second one to not break anything. Signed-off-by: Richard Weinberger rich...@nod.at Yeah, this atomic change stuff was added later, and we had not envisioned originally. Your patch adds robustness, but makes volume creation slower, which is probably not a problem. I've added a small comment and pushed it, thanks! Artem Hi Artem and Richard, We are using atomic operation, leb_change(), for master_node in ubifs-level. We use two lebs for master_node even if they are changed with atomic operation. I think volume_table and master_node play similar roles. Do you think changing VTBL record into one peb is OK? I just what to know if I missed something. Could you please take some time to explain that? Thanks very much~! Hu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 2014/10/24 11:33, hujianyang wrote:
>
>> @@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
>> int failures = 0;
>> struct ubi_device *ubi = u;
>>
>> -ubi_msg("background thread \"%s\" started, PID %d",
>> +ubi_msg(ubi, "background thread \"%s\" started, PID %d",
>> ubi->bgt_name, task_pid_nr(current));
>>
>> set_freezable();
>> for (;;) {
>> int err;
>>
>> -if (kthread_should_stop())
>> +if (kthread_should_stop()) {
>> +ubi_msg(ubi, "background thread \"%s\" should stop, PID
>> %d",
>> +ubi->bgt_name, task_pid_nr(current));
>> break;
>> +}
>>
>> if (try_to_freeze())
>> continue;
> @@ -470,8 +470,11 @@ struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device
> *ubi, int anchor)
> {
> struct ubi_wl_entry *e = NULL;
>
> - if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1))
> + if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1)) {
> + ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d,
> free_cnt=%d, reserved=%d",
> + anchor, ubi->free_count, ubi->beb_rsvd_pebs);
> goto out;
> + }
>
> if (anchor)
> e = find_anchor_wl_entry(>free);
Sorry, one of the adding messages should be this~!
>
>> @@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
>> int failures = 0;
>> struct ubi_device *ubi = u;
>>
>> -ubi_msg("background thread \"%s\" started, PID %d",
>> +ubi_msg(ubi, "background thread \"%s\" started, PID %d",
>> ubi->bgt_name, task_pid_nr(current));
>>
>> set_freezable();
>> for (;;) {
>> int err;
>>
>> -if (kthread_should_stop())
>> +if (kthread_should_stop()) {
>> +ubi_msg(ubi, "background thread \"%s\" should stop, PID
>> %d",
>> +ubi->bgt_name, task_pid_nr(current));
>> break;
>> +}
>>
>> if (try_to_freeze())
>> continue;
>
> Here are two new adding messages. Maybe a separate patch is better? Just a
> suggestion.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
Hi Tanya,
When I was trying to push this patch to my product, I reviewed this patch
and found some small problems. I wish it's not too late to report these.
The patch I get from linux-ubifs.git is amended a bit by Artem. I'd like to
quote your V5 patch for simplification. Some line numbers may mismatching.
> @@ -1408,20 +1416,20 @@ static int __init ubi_mtd_param_parse(const char
> *val, struct kernel_param *kp)
> return -EINVAL;
>
> if (mtd_devs == UBI_MAX_DEVICES) {
> - ubi_err("too many parameters, max. is %d\n",
> + pr_err("UBI error: too many parameters, max. is %d\n",
> UBI_MAX_DEVICES);
> return -EINVAL;
> }
>
> len = strnlen(val, MTD_PARAM_LEN_MAX);
> if (len == MTD_PARAM_LEN_MAX) {
> - ubi_err("parameter \"%s\" is too long, max. is %d\n",
> + pr_err("UBI error: parameter \"%s\" is too long, max. is %d\n",
> val, MTD_PARAM_LEN_MAX);
> return -EINVAL;
> }
>
> if (len == 0) {
> - pr_warn("UBI warning: empty 'mtd=' parameter - ignored\n");
> + pr_err("UBI warning: empty 'mtd=' parameter - ignored\n");
> return 0;
> }
Why the last 'pr_warn()' need to be changed into 'pr_err()'? I looked up your
V1 and V2 patches, I think it's not your purpose.
> @@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
> pnum, int ec)
>
> /**
> * validate_vid_hdr - check volume identifier header.
> + * @ubi: UBI device description object
> * @vid_hdr: the volume identifier header to check
> * @av: information about the volume this logical eraseblock belongs to
> * @pnum: physical eraseblock number the VID header came from
> @@ -48,13 +48,14 @@
>
> /**
> * get_exclusive - get exclusive access to an UBI volume.
> + * @ubi: UBI device description object
> * @desc: volume descriptor
> *
> * This function changes UBI volume open mode to "exclusive". Returns
> previous
> * mode value (positive integer) in case of success and a negative error code
> * in case of failure.
> */
> @@ -660,13 +660,14 @@ static int init_volumes(struct ubi_device *ubi,
>
> /**
> * check_av - check volume attaching information.
> + * @ubi: UBI device description object
> * @vol: UBI volume description object
> * @av: volume attaching information
> *
> * This function returns zero if the volume attaching information is
> consistent
> * to the data read from the volume tabla, and %-EINVAL if not.
> */
> -static int check_av(const struct ubi_volume *vol,
> +static int check_av(const struct ubi_device *ubi, const struct ubi_volume
> *vol,
> const struct ubi_ainf_volume *av)
> {
> int err;
This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device'
from
'ubi_volume'. So I think it's because when we call these functions, the '->ubi'
pointer of 'ubi_volume' is not initialized, am I right? This patch use
'vol->ubi'
to indicate a 'struct ubi_device *' pointer in some places, I think you are sure
of using them.
> @@ -1010,28 +1015,28 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
> ubi_num,
> ubi->bgt_thread = kthread_create(ubi_thread, ubi, "%s", ubi->bgt_name);
> if (IS_ERR(ubi->bgt_thread)) {
> err = PTR_ERR(ubi->bgt_thread);
> - ubi_err("cannot spawn \"%s\", error %d", ubi->bgt_name,
> - err);
> + ubi_err(ubi, "cannot spawn \"%s\", error %d",
> + ubi->bgt_name, err);
> goto out_debugfs;
> }
>
> - ubi_msg("attached mtd%d (name \"%s\", size %llu MiB) to ubi%d",
> - mtd->index, mtd->name, ubi->flash_size >> 20, ubi_num);
> - ubi_msg("PEB size: %d bytes (%d KiB), LEB size: %d bytes",
> + ubi_msg(ubi, "attached mtd%d (name \"%s\", size %llu MiB)",
> + mtd->index, mtd->name, ubi->flash_size >> 20);
> + ubi_msg(ubi, "PEB size: %d bytes (%d KiB), LEB size: %d bytes",
> ubi->peb_size, ubi->peb_size >> 10, ubi->leb_size);
We have the parameter 'ubi_num' for log in some functions like
'ubi_attach_mtd_dev'
before. This patch remove 'ubi_num' in upper changes but keep it in other
changes.
Do we have a discussed rule to deal with this situation? It's not a big problem~
> @@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
> int failures = 0;
> struct ubi_device *ubi = u;
>
> - ubi_msg("background thread \"%s\" started, PID %d",
> + ubi_msg(ubi, "background thread \"%s\" started, PID %d",
> ubi->bgt_name, task_pid_nr(current));
>
> set_freezable();
> for (;;) {
> int err;
>
> - if (kthread_should_stop())
> + if (kthread_should_stop()) {
> + ubi_msg(ubi, "background thread \"%s\" should stop, PID
> %d",
> + ubi->bgt_name,
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
Hi Tanya,
When I was trying to push this patch to my product, I reviewed this patch
and found some small problems. I wish it's not too late to report these.
The patch I get from linux-ubifs.git is amended a bit by Artem. I'd like to
quote your V5 patch for simplification. Some line numbers may mismatching.
@@ -1408,20 +1416,20 @@ static int __init ubi_mtd_param_parse(const char
*val, struct kernel_param *kp)
return -EINVAL;
if (mtd_devs == UBI_MAX_DEVICES) {
- ubi_err(too many parameters, max. is %d\n,
+ pr_err(UBI error: too many parameters, max. is %d\n,
UBI_MAX_DEVICES);
return -EINVAL;
}
len = strnlen(val, MTD_PARAM_LEN_MAX);
if (len == MTD_PARAM_LEN_MAX) {
- ubi_err(parameter \%s\ is too long, max. is %d\n,
+ pr_err(UBI error: parameter \%s\ is too long, max. is %d\n,
val, MTD_PARAM_LEN_MAX);
return -EINVAL;
}
if (len == 0) {
- pr_warn(UBI warning: empty 'mtd=' parameter - ignored\n);
+ pr_err(UBI warning: empty 'mtd=' parameter - ignored\n);
return 0;
}
Why the last 'pr_warn()' need to be changed into 'pr_err()'? I looked up your
V1 and V2 patches, I think it's not your purpose.
@@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int
pnum, int ec)
/**
* validate_vid_hdr - check volume identifier header.
+ * @ubi: UBI device description object
* @vid_hdr: the volume identifier header to check
* @av: information about the volume this logical eraseblock belongs to
* @pnum: physical eraseblock number the VID header came from
@@ -48,13 +48,14 @@
/**
* get_exclusive - get exclusive access to an UBI volume.
+ * @ubi: UBI device description object
* @desc: volume descriptor
*
* This function changes UBI volume open mode to exclusive. Returns
previous
* mode value (positive integer) in case of success and a negative error code
* in case of failure.
*/
@@ -660,13 +660,14 @@ static int init_volumes(struct ubi_device *ubi,
/**
* check_av - check volume attaching information.
+ * @ubi: UBI device description object
* @vol: UBI volume description object
* @av: volume attaching information
*
* This function returns zero if the volume attaching information is
consistent
* to the data read from the volume tabla, and %-EINVAL if not.
*/
-static int check_av(const struct ubi_volume *vol,
+static int check_av(const struct ubi_device *ubi, const struct ubi_volume
*vol,
const struct ubi_ainf_volume *av)
{
int err;
This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device'
from
'ubi_volume'. So I think it's because when we call these functions, the '-ubi'
pointer of 'ubi_volume' is not initialized, am I right? This patch use
'vol-ubi'
to indicate a 'struct ubi_device *' pointer in some places, I think you are sure
of using them.
@@ -1010,28 +1015,28 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
ubi_num,
ubi-bgt_thread = kthread_create(ubi_thread, ubi, %s, ubi-bgt_name);
if (IS_ERR(ubi-bgt_thread)) {
err = PTR_ERR(ubi-bgt_thread);
- ubi_err(cannot spawn \%s\, error %d, ubi-bgt_name,
- err);
+ ubi_err(ubi, cannot spawn \%s\, error %d,
+ ubi-bgt_name, err);
goto out_debugfs;
}
- ubi_msg(attached mtd%d (name \%s\, size %llu MiB) to ubi%d,
- mtd-index, mtd-name, ubi-flash_size 20, ubi_num);
- ubi_msg(PEB size: %d bytes (%d KiB), LEB size: %d bytes,
+ ubi_msg(ubi, attached mtd%d (name \%s\, size %llu MiB),
+ mtd-index, mtd-name, ubi-flash_size 20);
+ ubi_msg(ubi, PEB size: %d bytes (%d KiB), LEB size: %d bytes,
ubi-peb_size, ubi-peb_size 10, ubi-leb_size);
We have the parameter 'ubi_num' for log in some functions like
'ubi_attach_mtd_dev'
before. This patch remove 'ubi_num' in upper changes but keep it in other
changes.
Do we have a discussed rule to deal with this situation? It's not a big problem~
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
- ubi_msg(background thread \%s\ started, PID %d,
+ ubi_msg(ubi, background thread \%s\ started, PID %d,
ubi-bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
- if (kthread_should_stop())
+ if (kthread_should_stop()) {
+ ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
+ ubi-bgt_name, task_pid_nr(current));
break;
+ }
if (try_to_freeze())
continue;
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
On 2014/10/24 11:33, hujianyang wrote:
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
-ubi_msg(background thread \%s\ started, PID %d,
+ubi_msg(ubi, background thread \%s\ started, PID %d,
ubi-bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
-if (kthread_should_stop())
+if (kthread_should_stop()) {
+ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
+ubi-bgt_name, task_pid_nr(current));
break;
+}
if (try_to_freeze())
continue;
@@ -470,8 +470,11 @@ struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device
*ubi, int anchor)
{
struct ubi_wl_entry *e = NULL;
- if (!ubi-free.rb_node || (ubi-free_count - ubi-beb_rsvd_pebs 1))
+ if (!ubi-free.rb_node || (ubi-free_count - ubi-beb_rsvd_pebs 1)) {
+ ubi_warn(ubi, Can't get peb for fastmap:anchor=%d,
free_cnt=%d, reserved=%d,
+ anchor, ubi-free_count, ubi-beb_rsvd_pebs);
goto out;
+ }
if (anchor)
e = find_anchor_wl_entry(ubi-free);
Sorry, one of the adding messages should be this~!
@@ -1798,15 +1803,18 @@ int ubi_thread(void *u)
int failures = 0;
struct ubi_device *ubi = u;
-ubi_msg(background thread \%s\ started, PID %d,
+ubi_msg(ubi, background thread \%s\ started, PID %d,
ubi-bgt_name, task_pid_nr(current));
set_freezable();
for (;;) {
int err;
-if (kthread_should_stop())
+if (kthread_should_stop()) {
+ubi_msg(ubi, background thread \%s\ should stop, PID
%d,
+ubi-bgt_name, task_pid_nr(current));
break;
+}
if (try_to_freeze())
continue;
Here are two new adding messages. Maybe a separate patch is better? Just a
suggestion.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
It seems useful~! I'd like to do some test with it~ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
It seems useful~! I'd like to do some test with it~ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] audit: vfs: fix audit records error when write to a file
>
> Could you add me to your Cc: list on this thread, please? I'm
> interested in the outcome... Thanks!
>
Hi Richard,
I've resend a v2 patch and now quote it in the end of this mail
for you.
I'm sorry to say the previously work of mine seems useless. Moving
audit_inode() to the O_CREAT case in lookup_open() may miss some
conditions when we create a file.
fs/namei.c lookup_open() line 2828
"""
if ((nd->flags & LOOKUP_OPEN) && dir_inode->i_op->atomic_open) {
return atomic_open(nd, dentry, path, file, op, got_write,
need_lookup, opened);
}
"""
We will miss this branch and other potential cases if we apply my
patch.
There is something wrong with audit, I think. But I don't know where.
One good way to solve this problem is go back to commit bfcec708 as
Jeff mentioned in commit 33e2208a and find the reason why the audit
of create operations is wrong. Do we have another way to fix it than
changing the third parameter of audit_inode() to LOOKUP_PARENT which
lead the audit of write confused?
I don't know much about audit. One of my colleagues is dealing with
this problem now. I'm very glad if someone else could help us.
Thanks~!
Hu
On 2014/9/9 10:34, hujianyang wrote:
> Changes from v1:
>
>* Move audit_inode() to the beginning of O_CREAT case in
> lookup_open() to avoid missing audit for ROFS error. This
> lack is spotted by Jeff Layton
>
> commit 33e2208acfc1
>
> audit: vfs: fix audit_inode call in O_CREAT case of do_last
>
> fix a regression in auditing of open(..., O_CREAT) syscalls but
> introduce a new problem which lead the records of write operation
> confusion.
>
> This error can be reproduced by these steps:
>
> touch /etc/test
> echo "-w /etc/test" >>/etc/audit/audit.rules
> /etc/init.d/auditd restart
>
> echo "abc" >> /etc/test
>
> audit_name records are:
>
> type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097
> dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
> type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
> dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
> dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
>
> but if we revert commit 33e2208acfc1, records are correct:
>
> type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275
> dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
>
> We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
> of do_last() because this branch don't really know if vfs need to
> create a new file. There is no need to do vfs_create() if we open
> an existing file with O_CREAT flag and write to it. But this
> audit_inode() in O_CREAT case will record a msg as we create a new
> file and confuse the records of write.
>
> This patch moves the audit for create operation to where a file
> really need to be created, the O_CREAT case in lookup_open().
> We have to add the pointer of struct filename as a parameter of
> lookup_open(). By doing this, the records of both create and write
> are correct.
>
> Signed-off-by: hujianyang
> ---
> fs/namei.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index a996bb4..ca4a831 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2808,7 +2808,8 @@ looked_up:
> static int lookup_open(struct nameidata *nd, struct path *path,
> struct file *file,
> const struct open_flags *op,
> - bool got_write, int *opened)
> + bool got_write, int *opened,
> + struct filename *name)
> {
> struct dentry *dir = nd->path.dentry;
> struct inode *dir_inode = dir->d_inode;
> @@ -2841,6 +2842,8 @@ static int lookup_open(struct nameidata *nd, struct
> path *path,
> /* Negative dentry, just create the file */
> if (!dentry->d_inode && (op->open_flag & O_CREAT)) {
> umode_t mode = op->mode;
> +
> + audit_inode(name, dir, LOOKUP_PARENT);
> if (!IS_POSIXACL(dir->d_inode))
> mode &= ~current_umask();
> /*
> @@ -2926,7 +2929,6 @@ static int do_last(struct nameidata *nd, struct path
> *path,
> if (error)
> return error;
>
> - audit_inode(name, dir, LOOKUP_PARENT);
> error = -EISDIR;
> /* trailing
Re: [PATCH v2] audit: vfs: fix audit records error when write to a file
Could you add me to your Cc: list on this thread, please? I'm
interested in the outcome... Thanks!
Hi Richard,
I've resend a v2 patch and now quote it in the end of this mail
for you.
I'm sorry to say the previously work of mine seems useless. Moving
audit_inode() to the O_CREAT case in lookup_open() may miss some
conditions when we create a file.
fs/namei.c lookup_open() line 2828
if ((nd-flags LOOKUP_OPEN) dir_inode-i_op-atomic_open) {
return atomic_open(nd, dentry, path, file, op, got_write,
need_lookup, opened);
}
We will miss this branch and other potential cases if we apply my
patch.
There is something wrong with audit, I think. But I don't know where.
One good way to solve this problem is go back to commit bfcec708 as
Jeff mentioned in commit 33e2208a and find the reason why the audit
of create operations is wrong. Do we have another way to fix it than
changing the third parameter of audit_inode() to LOOKUP_PARENT which
lead the audit of write confused?
I don't know much about audit. One of my colleagues is dealing with
this problem now. I'm very glad if someone else could help us.
Thanks~!
Hu
On 2014/9/9 10:34, hujianyang wrote:
Changes from v1:
* Move audit_inode() to the beginning of O_CREAT case in
lookup_open() to avoid missing audit for ROFS error. This
lack is spotted by Jeff Layton jeff.lay...@primarydata.com
commit 33e2208acfc1
audit: vfs: fix audit_inode call in O_CREAT case of do_last
fix a regression in auditing of open(..., O_CREAT) syscalls but
introduce a new problem which lead the records of write operation
confusion.
This error can be reproduced by these steps:
touch /etc/test
echo -w /etc/test /etc/audit/audit.rules
/etc/init.d/auditd restart
echo abc /etc/test
audit_name records are:
type=PATH msg=audit(1409764556.196:67): item=0 name=/etc/ inode=5097
dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
but if we revert commit 33e2208acfc1, records are correct:
type=PATH msg=audit(1409763058.192:219): item=0 name=/etc/test inode=1275
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.
This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.
Signed-off-by: hujianyang hujiany...@huawei.com
---
fs/namei.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..ca4a831 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
static int lookup_open(struct nameidata *nd, struct path *path,
struct file *file,
const struct open_flags *op,
- bool got_write, int *opened)
+ bool got_write, int *opened,
+ struct filename *name)
{
struct dentry *dir = nd-path.dentry;
struct inode *dir_inode = dir-d_inode;
@@ -2841,6 +2842,8 @@ static int lookup_open(struct nameidata *nd, struct
path *path,
/* Negative dentry, just create the file */
if (!dentry-d_inode (op-open_flag O_CREAT)) {
umode_t mode = op-mode;
+
+ audit_inode(name, dir, LOOKUP_PARENT);
if (!IS_POSIXACL(dir-d_inode))
mode = ~current_umask();
/*
@@ -2926,7 +2929,6 @@ static int do_last(struct nameidata *nd, struct path
*path,
if (error)
return error;
- audit_inode(name, dir, LOOKUP_PARENT);
error = -EISDIR;
/* trailing slashes? */
if (nd-last.name[nd-last.len])
@@ -2945,7 +2947,7 @@ retry_lookup:
*/
}
mutex_lock(dir-d_inode-i_mutex);
- error = lookup_open(nd, path, file, op, got_write, opened);
+ error = lookup_open(nd, path, file, op, got_write, opened, name);
mutex_unlock(dir-d_inode-i_mutex);
if (error = 0) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord
[PATCH v2] audit: vfs: fix audit records error when write to a file
Changes from v1:
* Move audit_inode() to the beginning of O_CREAT case in
lookup_open() to avoid missing audit for ROFS error. This
lack is spotted by Jeff Layton
commit 33e2208acfc1
audit: vfs: fix audit_inode call in O_CREAT case of do_last
fix a regression in auditing of open(..., O_CREAT) syscalls but
introduce a new problem which lead the records of write operation
confusion.
This error can be reproduced by these steps:
touch /etc/test
echo "-w /etc/test" >>/etc/audit/audit.rules
/etc/init.d/auditd restart
echo "abc" >> /etc/test
audit_name records are:
type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097
dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
but if we revert commit 33e2208acfc1, records are correct:
type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.
This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.
Signed-off-by: hujianyang
---
fs/namei.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..ca4a831 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
static int lookup_open(struct nameidata *nd, struct path *path,
struct file *file,
const struct open_flags *op,
- bool got_write, int *opened)
+ bool got_write, int *opened,
+ struct filename *name)
{
struct dentry *dir = nd->path.dentry;
struct inode *dir_inode = dir->d_inode;
@@ -2841,6 +2842,8 @@ static int lookup_open(struct nameidata *nd, struct path
*path,
/* Negative dentry, just create the file */
if (!dentry->d_inode && (op->open_flag & O_CREAT)) {
umode_t mode = op->mode;
+
+ audit_inode(name, dir, LOOKUP_PARENT);
if (!IS_POSIXACL(dir->d_inode))
mode &= ~current_umask();
/*
@@ -2926,7 +2929,6 @@ static int do_last(struct nameidata *nd, struct path
*path,
if (error)
return error;
- audit_inode(name, dir, LOOKUP_PARENT);
error = -EISDIR;
/* trailing slashes? */
if (nd->last.name[nd->last.len])
@@ -2945,7 +2947,7 @@ retry_lookup:
*/
}
mutex_lock(>d_inode->i_mutex);
- error = lookup_open(nd, path, file, op, got_write, opened);
+ error = lookup_open(nd, path, file, op, got_write, opened, name);
mutex_unlock(>d_inode->i_mutex);
if (error <= 0) {
--
1.8.5.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] audit: vfs: fix audit records error when write to a file
Changes from v1:
* Move audit_inode() to the beginning of O_CREAT case in
lookup_open() to avoid missing audit for ROFS error. This
lack is spotted by Jeff Layton jeff.lay...@primarydata.com
commit 33e2208acfc1
audit: vfs: fix audit_inode call in O_CREAT case of do_last
fix a regression in auditing of open(..., O_CREAT) syscalls but
introduce a new problem which lead the records of write operation
confusion.
This error can be reproduced by these steps:
touch /etc/test
echo -w /etc/test /etc/audit/audit.rules
/etc/init.d/auditd restart
echo abc /etc/test
audit_name records are:
type=PATH msg=audit(1409764556.196:67): item=0 name=/etc/ inode=5097
dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
but if we revert commit 33e2208acfc1, records are correct:
type=PATH msg=audit(1409763058.192:219): item=0 name=/etc/test inode=1275
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.
This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.
Signed-off-by: hujianyang hujiany...@huawei.com
---
fs/namei.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..ca4a831 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
static int lookup_open(struct nameidata *nd, struct path *path,
struct file *file,
const struct open_flags *op,
- bool got_write, int *opened)
+ bool got_write, int *opened,
+ struct filename *name)
{
struct dentry *dir = nd-path.dentry;
struct inode *dir_inode = dir-d_inode;
@@ -2841,6 +2842,8 @@ static int lookup_open(struct nameidata *nd, struct path
*path,
/* Negative dentry, just create the file */
if (!dentry-d_inode (op-open_flag O_CREAT)) {
umode_t mode = op-mode;
+
+ audit_inode(name, dir, LOOKUP_PARENT);
if (!IS_POSIXACL(dir-d_inode))
mode = ~current_umask();
/*
@@ -2926,7 +2929,6 @@ static int do_last(struct nameidata *nd, struct path
*path,
if (error)
return error;
- audit_inode(name, dir, LOOKUP_PARENT);
error = -EISDIR;
/* trailing slashes? */
if (nd-last.name[nd-last.len])
@@ -2945,7 +2947,7 @@ retry_lookup:
*/
}
mutex_lock(dir-d_inode-i_mutex);
- error = lookup_open(nd, path, file, op, got_write, opened);
+ error = lookup_open(nd, path, file, op, got_write, opened, name);
mutex_unlock(dir-d_inode-i_mutex);
if (error = 0) {
--
1.8.5.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] audit: vfs: fix audit records error when write to a file
commit 33e2208acfc1
audit: vfs: fix audit_inode call in O_CREAT case of do_last
fix a regression in auditing of open(..., O_CREAT) syscalls but
import a new problem which lead the records of write operation
confusion.
This error can be reproduced by these steps:
touch /etc/test
echo "-w /etc/test" >>/etc/audit/audit.rules
/etc/init.d/auditd restart
echo "abc" >> /etc/test
audit_name records are:
type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097
dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
but if we revert commit 33e2208acfc1, records are correct:
type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.
This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.
Signed-off-by: hujianyang
---
fs/namei.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..0bc7734 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
static int lookup_open(struct nameidata *nd, struct path *path,
struct file *file,
const struct open_flags *op,
- bool got_write, int *opened)
+ bool got_write, int *opened,
+ struct filename *name)
{
struct dentry *dir = nd->path.dentry;
struct inode *dir_inode = dir->d_inode;
@@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path
*path,
error = -EROFS;
goto out_dput;
}
+
+ audit_inode(name, dir, LOOKUP_PARENT);
+
*opened |= FILE_CREATED;
error = security_path_mknod(>path, dentry, mode, 0);
if (error)
@@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path
*path,
if (error)
return error;
- audit_inode(name, dir, LOOKUP_PARENT);
error = -EISDIR;
/* trailing slashes? */
if (nd->last.name[nd->last.len])
@@ -2945,7 +2948,7 @@ retry_lookup:
*/
}
mutex_lock(>d_inode->i_mutex);
- error = lookup_open(nd, path, file, op, got_write, opened);
+ error = lookup_open(nd, path, file, op, got_write, opened, name);
mutex_unlock(>d_inode->i_mutex);
if (error <= 0) {
--
1.8.5.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] audit: vfs: fix audit records error when write to a file
commit 33e2208acfc1
audit: vfs: fix audit_inode call in O_CREAT case of do_last
fix a regression in auditing of open(..., O_CREAT) syscalls but
import a new problem which lead the records of write operation
confusion.
This error can be reproduced by these steps:
touch /etc/test
echo -w /etc/test /etc/audit/audit.rules
/etc/init.d/auditd restart
echo abc /etc/test
audit_name records are:
type=PATH msg=audit(1409764556.196:67): item=0 name=/etc/ inode=5097
dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
but if we revert commit 33e2208acfc1, records are correct:
type=PATH msg=audit(1409763058.192:219): item=0 name=/etc/test inode=1275
dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.
This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.
Signed-off-by: hujianyang hujiany...@huawei.com
---
fs/namei.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..0bc7734 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
static int lookup_open(struct nameidata *nd, struct path *path,
struct file *file,
const struct open_flags *op,
- bool got_write, int *opened)
+ bool got_write, int *opened,
+ struct filename *name)
{
struct dentry *dir = nd-path.dentry;
struct inode *dir_inode = dir-d_inode;
@@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path
*path,
error = -EROFS;
goto out_dput;
}
+
+ audit_inode(name, dir, LOOKUP_PARENT);
+
*opened |= FILE_CREATED;
error = security_path_mknod(nd-path, dentry, mode, 0);
if (error)
@@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path
*path,
if (error)
return error;
- audit_inode(name, dir, LOOKUP_PARENT);
error = -EISDIR;
/* trailing slashes? */
if (nd-last.name[nd-last.len])
@@ -2945,7 +2948,7 @@ retry_lookup:
*/
}
mutex_lock(dir-d_inode-i_mutex);
- error = lookup_open(nd, path, file, op, got_write, opened);
+ error = lookup_open(nd, path, file, op, got_write, opened, name);
mutex_unlock(dir-d_inode-i_mutex);
if (error = 0) {
--
1.8.5.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
