* [PATCH]jffs2:bugfix for should not appeared directory hard link
@ 2014-10-24 9:26 liu.song11
2014-11-05 11:45 ` Brian Norris
2015-02-12 13:43 ` David Woodhouse
0 siblings, 2 replies; 13+ messages in thread
From: liu.song11 @ 2014-10-24 9:26 UTC (permalink / raw)
Cc: jiang.xuexin, linux-mtd, cui.yunfeng, wang.bo116, dwmw2,
deng.chao1
hi everyone
When jffs2 with summary, during the mounting period, in
jffs2_build_remove_unlinked_inode will
handle unlinke inode, but this will cause unlinked directory's deletion
dirent erased before
it's obsoleted dirent.So if the directory has renamed, then could cause a
hard link when we
remount jffs2.
We can stable reproduce the hard link only by following process.
1. we mount jffs2 in /mnt
2. create directories /mnt/SW1/, /mnt/SW2/
3. create directory /mnt/old/
4. rename /mnt/SW1/ to /mnt/old/SW1/, /mnt/SW2/ to /mnt/old/SW2/
5. do some data write. don't touch these directories.
6. rename /mnt/old/SW1/ to /mnt/SW1/, /mnt/old/SW2/ to /mnt/SW2/
7. delete /mnt/old/
do above process 3 - 7 several times, then reboot, during the mounting
process, we could found
/mnt/SW1/ and /mnt/old/SW1/ become hard link,/mnt/SW2/ and /mnt/old/SW2/
become hard link. So we
do some job to fix this bug.We have tested this patch and the hard link
error will never occur.
Thanks!
Best Regards
------------------------------------------------------------------------------------------------
diff --git a/build.c b/build.c
index a3750f9..ad7b8a6 100644
--- a/build.c
+++ b/build.c
@@ -205,7 +205,9 @@ static void jffs2_build_remove_unlinked_inode(struct
jffs2_sb_info *c,
while (raw != (void *)ic) {
struct jffs2_raw_node_ref *next = raw->next_in_ino;
dbg_fsbuild("obsoleting node at 0x%08x\n",
ref_offset(raw));
- jffs2_mark_node_obsolete(c, raw);
+ /* prevent make deletion dirent obsolete directly*/
+ if (ref_flags(raw) != REF_NORMAL)
+ jffs2_mark_node_obsolete(c, raw);
raw = next;
}
diff --git a/fs.c b/fs.c
index 601afd1..ac3abe0 100644
--- a/fs.c
+++ b/fs.c
@@ -657,6 +657,7 @@ struct jffs2_inode_info *jffs2_gc_fetch_inode(struct
jffs2_sb_info *c,
ic->ino, ic->state);
sleep_on_spinunlock(&c->inocache_wq,
&c->inocache_lock);
} else {
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
}
diff --git a/gc.c b/gc.c
index 5a2dec2..3a0f84a 100644
--- a/gc.c
+++ b/gc.c
@@ -39,6 +39,14 @@ static int jffs2_garbage_collect_dnode(struct
jffs2_sb_info *c, struct jffs2_era
uint32_t start, uint32_t end);
static int jffs2_garbage_collect_live(struct jffs2_sb_info *c, struct
jffs2_eraseblock *jeb,
struct jffs2_raw_node_ref *raw, struct
jffs2_inode_info *f);
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info
*c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd);
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref
*raw_suspect,
+ struct jffs2_inode_cache *ic);
/* Called with erase_completion_lock held */
static struct jffs2_eraseblock *jffs2_find_gc_block(struct jffs2_sb_info
*c)
@@ -169,6 +177,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
if (!ic->pino_nlink) {
jffs2_dbg(1, "Skipping check of ino #%d with
nlink/pino zero\n",
ic->ino);
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
jffs2_xattr_delete_inode(c, ic);
continue;
@@ -358,9 +367,14 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
case INO_STATE_PRESENT:
/* It's in-core. GC must iget() it. */
+ case INO_STATE_DELETION:
+ /* Unlinked node. GC must check deletion dirent. */
break;
case INO_STATE_UNCHECKED:
+ ic->state = INO_STATE_DELETION;
+ break;
+
case INO_STATE_CHECKING:
case INO_STATE_GC:
/* Should never happen. We should have finished checking
@@ -431,19 +445,26 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
nlink = ic->pino_nlink;
spin_unlock(&c->inocache_lock);
- f = jffs2_gc_fetch_inode(c, inum, !nlink);
+ if (ic->state == INO_STATE_DELETION)
+ f = NULL;
+ else
+ f = jffs2_gc_fetch_inode(c, inum, !nlink);
+
if (IS_ERR(f)) {
ret = PTR_ERR(f);
goto release_sem;
}
- if (!f) {
+ if (!f && ic->state != INO_STATE_DELETION) {
ret = 0;
goto release_sem;
}
- ret = jffs2_garbage_collect_live(c, jeb, raw, f);
-
- jffs2_gc_release_inode(c, f);
+ if (ic->state == INO_STATE_DELETION) {
+ ret = jffs2_garbage_collect_unlinked_dirent(c, jeb, raw,
ic);
+ } else {
+ ret = jffs2_garbage_collect_live(c, jeb, raw, f);
+ jffs2_gc_release_inode(c, f);
+ }
test_gcnode:
if (jeb->dirty_size == gcblock_dirty &&
!ref_obsolete(jeb->gc_node)) {
@@ -990,6 +1011,236 @@ static int
jffs2_garbage_collect_deletion_dirent(struct jffs2_sb_info *c, struct
return 0;
}
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info
*c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd)
+{
+ size_t retlen;
+ int ret;
+ uint32_t phys_ofs, alloclen;
+ uint32_t rawlen;
+ int retried = 0;
+
+ jffs2_dbg(1, "Going to GC REF_DELETION node at 0x%08x\n",
+ ref_offset(raw));
+ alloclen = rawlen = ref_totlen(c, c->gcblock, raw);
+
+ /* Ask for a small amount of space (or the totlen if smaller)
because we
+ don't want to force wastage of the end of a block if splitting
would
+ work. */
+ if (ic && alloclen > sizeof(struct jffs2_raw_inode) +
JFFS2_MIN_DATA_LEN)
+ alloclen = sizeof(struct jffs2_raw_inode) +
JFFS2_MIN_DATA_LEN;
+
+ ret = jffs2_reserve_space_gc(c, alloclen, &alloclen, rawlen);
+
+ if (ret)
+ goto out_node;
+
+ if (alloclen < rawlen) {
+ ret = -EBADFD;
+ goto out_node;
+ }
+
+ retry:
+ phys_ofs = write_ofs(c);
+
+ ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)rd);
+
+ if (ret || (retlen != rawlen)) {
+ pr_notice("Write of %d bytes at 0x%08x failed. returned
%d, retlen %zd\n",
+ rawlen, phys_ofs, ret, retlen);
+ if (retlen) {
+ jffs2_add_physical_node_ref(c, phys_ofs |
REF_OBSOLETE, rawlen, NULL);
+ } else {
+ pr_notice("Not marking the space at 0x%08x as
dirty because the flash driver returned retlen zero\n",
+ phys_ofs);
+ }
+ if (!retried) {
+ /* Try to reallocate space and retry */
+ uint32_t dummy;
+ struct jffs2_eraseblock *jeb = &c->blocks[phys_ofs
/ c->sector_size];
+
+ retried = 1;
+
+ jffs2_dbg(1, "Retrying failed write of
REF_DELETION node.\n");
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ ret = jffs2_reserve_space_gc(c, rawlen, &dummy,
rawlen);
+
+ if (!ret) {
+ jffs2_dbg(1, "Allocated space at 0x%08x to
retry failed write.\n",
+ phys_ofs);
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ goto retry;
+ }
+ jffs2_dbg(1, "Failed to allocate space to retry
failed write: %d!\n",
+ ret);
+ }
+
+ if (!ret)
+ ret = -EIO;
+ goto out_node;
+ }
+ jffs2_add_physical_node_ref(c, phys_ofs | REF_NORMAL, rawlen, ic);
+
+ jffs2_mark_node_obsolete(c, raw);
+ jffs2_dbg(1, "WHEEE! GC REF_DELETION node at 0x%08x succeeded\n",
+ ref_offset(raw));
+ out_node:
+ kfree(rd);
+ return ret;
+}
+
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref *raw_suspect,
+ struct jffs2_inode_cache *ic)
+{
+ struct jffs2_full_dirent *fd = NULL;
+ struct jffs2_raw_dirent *rd = NULL;
+ struct jffs2_raw_dirent *rd_suspect = NULL;
+
+ if (!jffs2_can_mark_obsolete(c)) {
+ int ret;
+ int name_len;
+ size_t retlen;
+ uint32_t checkedlen;
+ uint32_t name_crc;
+ uint32_t raw_len;
+ struct jffs2_raw_node_ref *raw;
+
+ raw_len = ref_totlen(c, jeb, raw_suspect);
+ rd_suspect = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd_suspect)
+ return -ENOMEM;
+
+ ret = jffs2_flash_read(c, ref_offset(raw_suspect),
+ raw_len, &retlen, (char *)rd_suspect);
+
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading deletion
dirent node at %08x\n",
+ __func__, ret, ref_offset(raw_suspect));
+ kfree(rd_suspect);
+ return -EIO;
+ }
+
+ if (je16_to_cpu(rd_suspect->nodetype) !=
JFFS2_NODETYPE_DIRENT) {
+ pr_warn("%s(): ofs(%08x) nodetype(%04x) is not
expected\n",
+ __func__, ref_offset(raw_suspect),
+ je16_to_cpu(rd_suspect->nodetype));
+ kfree(rd_suspect);
+ BUG();
+ }
+
+ if (je32_to_cpu(rd_suspect->ino)) {
+ jffs2_dbg(1, "%s(): ino %d not deletion dirent\n",
+ __func__, je32_to_cpu(rd_suspect->ino));
+ goto obsolete;
+ }
+
+ checkedlen = strnlen(rd_suspect->name, rd_suspect->nsize);
+ fd = jffs2_alloc_full_dirent(checkedlen+1);
+ if (!fd) {
+ kfree(rd_suspect);
+ return -ENOMEM;
+ }
+
+ memcpy(&fd->name, rd_suspect->name, checkedlen);
+ fd->name[checkedlen] = 0;
+ fd->raw = raw_suspect;
+ name_len = strlen(fd->name);
+ name_crc = crc32(0, fd->name, name_len);
+ rd = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd) {
+ kfree(rd_suspect);
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+
+ /* Prevent the erase code from nicking the obsolete node
refs
+ while we're looking at them. I really don't like this
extra lock but
+ can't see any alternative. Suggestions on a postcard to...
*/
+ mutex_lock(&c->erase_free_sem);
+ for (raw = ic->nodes; raw != (void *)ic; raw =
raw->next_in_ino) {
+
+ cond_resched();
+
+ /* We only care about obsolete ones */
+ if (!(ref_obsolete(raw)))
+ continue;
+
+ /* Any dirent with the same name is going to have
the same length... */
+ if (ref_totlen(c, NULL, raw) != raw_len)
+ continue;
+
+ /* Doesn't matter if there's one in the same erase
block. We're going to
+ delete it too at the same time. */
+ if (SECTOR_ADDR(raw->flash_offset) ==
SECTOR_ADDR(fd->raw->flash_offset))
+ continue;
+
+ jffs2_dbg(1, "Check potential deletion dirent at
%08x\n",
+ ref_offset(raw));
+
+ /* This is an obsolete node belonging to the same
directory, and it's of the right
+ length. We need to take a closer look...*/
+ ret = jffs2_flash_read(c, ref_offset(raw),
raw_len, &retlen, (char *)rd);
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading
obsolete node at %08x\n",
+ __func__, ret, ref_offset(raw));
+ /* If we can't read it, we don't need to
continue to obsolete it. Continue */
+ continue;
+ }
+ if (retlen != raw_len) {
+ pr_warn("%s(): Short read (%zd not %u)
reading header from obsolete node at %08x\n",
+ __func__, retlen, raw_len,
+ ref_offset(raw));
+ continue;
+ }
+
+ if (je16_to_cpu(rd->nodetype) !=
JFFS2_NODETYPE_DIRENT)
+ continue;
+
+ /* If the name CRC doesn't match, skip */
+ if (je32_to_cpu(rd->name_crc) != name_crc)
+ continue;
+
+ /* If the name length doesn't match, or it's
another deletion dirent, skip */
+ if (rd->nsize != name_len ||
!je32_to_cpu(rd->ino))
+ continue;
+
+ /* OK, check the actual name now */
+ if (memcmp(rd->name, fd->name, name_len))
+ continue;
+
+ /* OK. The name really does match. There really is
still an older node on
+ the flash which our deletion dirent obsoletes.
So we have to write out
+ a new deletion dirent to replace it */
+ mutex_unlock(&c->erase_free_sem);
+
+ jffs2_dbg(1, "Deletion dirent at %08x still
obsoletes real dirent \"%s\" at %08x for ino #%u\n",
+ ref_offset(fd->raw), fd->name,
+ ref_offset(raw), je32_to_cpu(rd->ino));
+ kfree(rd);
+ jffs2_free_full_dirent(fd);
+ return jffs2_garbage_collect_unlinked_deletion(c,
ic, raw_suspect, rd_suspect);
+ }
+ mutex_unlock(&c->erase_free_sem);
+ }
+obsolete:
+ jffs2_mark_node_obsolete(c, raw_suspect);
+ if (fd)
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ kfree(rd_suspect);
+ return 0;
+}
+
static int jffs2_garbage_collect_hole(struct jffs2_sb_info *c, struct
jffs2_eraseblock *jeb,
struct jffs2_inode_info *f, struct
jffs2_full_dnode *fn,
uint32_t start, uint32_t end)
diff --git a/nodelist.h b/nodelist.h
index fa35ff7..e8e282a 100644
--- a/nodelist.h
+++ b/nodelist.h
@@ -192,6 +192,7 @@ struct jffs2_inode_cache {
#define INO_STATE_GC 4 /* GCing a 'pristine' node */
#define INO_STATE_READING 5 /* In read_inode() */
#define INO_STATE_CLEARING 6 /* In clear_inode() */
+#define INO_STATE_DELETION 7 /* Unlinked inode */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate
xattr_ref */
diff --git a/summary.c b/summary.c
index c522d09..2488d50 100644
--- a/summary.c
+++ b/summary.c
@@ -470,17 +470,85 @@ static int jffs2_sum_process_sum_data(struct
jffs2_sb_info *c, struct jffs2_eras
return -ENOMEM;
}
- fd->raw = sum_link_node_ref(c, jeb,
je32_to_cpu(spd->offset) | REF_UNCHECKED,
- PAD(je32_to_cpu(spd->totlen)), ic);
-
+ fd->ino = je32_to_cpu(spd->ino);
+ if (fd->ino != 0) {
+ fd->raw = sum_link_node_ref(c,
jeb, je32_to_cpu(spd->offset) | REF_UNCHECKED,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ } else {
+ int ret;
+ size_t retlen;
+ uint32_t hdr_crc, crc;
+ struct jffs2_raw_dirent *rd;
+ uint32_t rd_totlen =
je32_to_cpu(spd->totlen);
+
+ rd = kmalloc(rd_totlen,
GFP_KERNEL);
+ if (!rd) {
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+ ret = jffs2_flash_read(c,
jeb->offset + je32_to_cpu(spd->offset),
+ rd_totlen, &retlen, (char
*)rd);
+ if (!ret && retlen != rd_totlen) {
+ pr_err("Dirent at %08x has
read problem. Aborting mount.\n",
+ jeb->offset +
je32_to_cpu(spd->offset));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ return -EIO;
+ }
+ hdr_crc = crc32(0, rd,
sizeof(struct jffs2_unknown_node)-4);
+ if (hdr_crc !=
je32_to_cpu(rd->hdr_crc)) {
+ pr_notice("%s(): Node at
0x%08x {0x%04x, 0x%04x, 0x%08x) has invalid CRC 0x%08x (calculated
0x%08x)\n",
+ __func__,
+ jeb->offset +
je32_to_cpu(spd->offset),
+ je16_to_cpu(rd->magic),
+ je16_to_cpu(rd->nodetype),
+ je32_to_cpu(rd->totlen),
+ je32_to_cpu(rd->hdr_crc),
+ hdr_crc);
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, rd, sizeof(*rd)-8);
+ if (crc !=
je32_to_cpu(rd->node_crc)) {
+ pr_notice("%s(): Node CRC
failed on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
+ __func__,
jeb->offset + je32_to_cpu(spd->offset),
+ je32_to_cpu(rd->node_crc), crc);
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, fd->name,
rd->nsize);
+ if (crc !=
je32_to_cpu(rd->name_crc)) {
+ pr_notice("%s(): Name CRC
failed on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
+ __func__,
jeb->offset + je32_to_cpu(spd->offset),
+ je32_to_cpu(rd->name_crc), crc);
+ jffs2_dbg(1, "Name for
which CRC failed is (now) '%s', ino #%d\n",
+ fd->name,
je32_to_cpu(rd->ino));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ kfree(rd);
+ fd->raw = sum_link_node_ref(c,
jeb, je32_to_cpu(spd->offset) | REF_NORMAL,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ }
fd->next = NULL;
fd->version = je32_to_cpu(spd->version);
- fd->ino = je32_to_cpu(spd->ino);
fd->nhash = full_name_hash(fd->name,
checkedlen);
fd->type = spd->type;
jffs2_add_fd_to_list(c, fd,
&ic->scan_dents);
-
+out:
*pseudo_random +=
je32_to_cpu(spd->version);
sp +=
JFFS2_SUMMARY_DIRENT_SIZE(spd->nsize);
diff --git a/write.c b/write.c
index b634de4..750257a 100644
--- a/write.c
+++ b/write.c
@@ -648,7 +648,7 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct
jffs2_inode_info *dir_f,
fd->name,
dead_f->inocache->ino);
}
- if (fd->raw)
+ if (fd->raw && fd->ino)
jffs2_mark_node_obsolete(c,
fd->raw);
jffs2_free_full_dirent(fd);
}
Signed-off-by: Liu Song <liu.song11@zte.com.cn>
--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-10-24 9:26 [PATCH]jffs2:bugfix for should not appeared directory hard link liu.song11
@ 2014-11-05 11:45 ` Brian Norris
2014-11-11 7:12 ` liu.song11
2014-11-12 6:45 ` Liu Song
2015-02-12 13:43 ` David Woodhouse
1 sibling, 2 replies; 13+ messages in thread
From: Brian Norris @ 2014-11-05 11:45 UTC (permalink / raw)
To: liu.song11
Cc: jiang.xuexin, linux-mtd, cui.yunfeng, wang.bo116, dwmw2,
deng.chao1
On Fri, Oct 24, 2014 at 05:26:05PM +0800, liu.song11@zte.com.cn wrote:
> hi everyone
Hi!
> When jffs2 with summary, during the mounting period, in
> jffs2_build_remove_unlinked_inode will
> handle unlinke inode, but this will cause unlinked directory's deletion
> dirent erased before
> it's obsoleted dirent.So if the directory has renamed, then could cause a
> hard link when we
> remount jffs2.
> We can stable reproduce the hard link only by following process.
> 1. we mount jffs2 in /mnt
> 2. create directories /mnt/SW1/, /mnt/SW2/
> 3. create directory /mnt/old/
> 4. rename /mnt/SW1/ to /mnt/old/SW1/, /mnt/SW2/ to /mnt/old/SW2/
> 5. do some data write. don't touch these directories.
> 6. rename /mnt/old/SW1/ to /mnt/SW1/, /mnt/old/SW2/ to /mnt/SW2/
> 7. delete /mnt/old/
>
> do above process 3 - 7 several times, then reboot, during the mounting
> process, we could found
> /mnt/SW1/ and /mnt/old/SW1/ become hard link,/mnt/SW2/ and /mnt/old/SW2/
> become hard link. So we
> do some job to fix this bug.We have tested this patch and the hard link
> error will never occur.
> Thanks!
> Best Regards
This only describes the problem, but it doesn't describe the fix at all.
Please provide a proper commit description.
Also, your patch is very big and line-wrapped, so it's very hard to
figure out what it's doing, and it won't apply to my git tree. Please
check your mailer settings.
All this is addressed in Documentation/SubmittingPatches, which I
suggest you read.
Regards,
Brian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-11-05 11:45 ` Brian Norris
@ 2014-11-11 7:12 ` liu.song11
2014-11-12 6:45 ` Liu Song
1 sibling, 0 replies; 13+ messages in thread
From: liu.song11 @ 2014-11-11 7:12 UTC (permalink / raw)
To: Brian Norris
Cc: cui.yunfeng, linux-mtd, jiang.xuexin, wang.bo116, dwmw2,
deng.chao1
Brian Norris <computersforpeace@gmail.com> wrote on 2014-11-05 19:45:00:
>
> This only describes the problem, but it doesn't describe the fix at all.
> Please provide a proper commit description.
>
> Also, your patch is very big and line-wrapped, so it's very hard to
> figure out what it's doing, and it won't apply to my git tree. Please
> check your mailer settings.
>
> All this is addressed in Documentation/SubmittingPatches, which I
> suggest you read.
>
> Regards,
> Brian
Hi, Brian
Thanks a lot for your attention and suggestions. I've checked the mail
settings and changed wrap line character number to a suitable value
which was only 70 and had made everything looked in a mess. Follow the
guide, patch had checked by checkpatch.pl, but there also had warnings
about line over 80 characters, I had no idea to fix that except remove the
code indentations, but i'm afraid that would make codes hard to read.
However, if there are any suggestions to fix this I would try.
The following is the description about this patch.
The patch is focus on jffs2 with summary. We check deletion dirent's crc
in "jffs2_sum_process_sum_data" so could make its ref flag REF_NORMAL.
After this process, all crc correct deletion dirents' ref flag are
REF_NORMAL then could separate from other nodes(REF_UNCHECKED).
We use this flag to protect unlinked deletion dirent directly turned to
obsoleted in "jffs2_build_remove_unlinked_inode", because deletion dirents
maybe still useful if it's obsoleted dirent remain in flash. These
unlinked deletion dirents will be checked whether still useful in gc time.
These unlinked deletion dirents in "jffs2_garbage_collect_pass" will skip
to check, because its "jffs2_inode_cache"'s pino_nlink is zero.
Then in "jffs2_garbage_collect_pass", unlinked deletion dirents'
"ic->state" is INO_STATE_UNCHECKED, but we had checked the crc during
scan. So we give
its a new state which is INO_STATE_DELETION. If gc find current node's
"ic->state" is INO_STATE_DELETION will know it's an unlinked deletion
dirent,
then will check whether its still useful in "
jffs2_garbage_collect_unlinked_dirent" which is similar to "
jffs2_garbage_collect_deletion_dirent" but use
different parameters. If the unlinked deletion dirent is still useful, we
copy it, else, we could safely mark it obsoleted.
In "jffs2_do_unlink", there also has the same problem, it will obsolete
the unlinked directory's child dirents which include deletion dirent, so
we
should also protect the deletion dirent.
The main purpose of this patch is to prevent unlinked deletion direct
directly turn to obsolete and handle it during gc time. We had do many
tests on this
patch and it worked properly, the directory hard link couldn't occured. We
had also test the deletion dirents' crc in scan time whether cost a lot,
the result is check 81809 deletion dirents' crc could within one second,
so it will not extend the mounting time.
Thanks
Best Regards
Liu Song
-------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/build.c b/build.c
index a3750f9..ad7b8a6 100644
--- a/build.c
+++ b/build.c
@@ -205,7 +205,9 @@ static void jffs2_build_remove_unlinked_inode(struct
jffs2_sb_info *c,
while (raw != (void *)ic) {
struct jffs2_raw_node_ref *next = raw->next_in_ino;
dbg_fsbuild("obsoleting node at 0x%08x\n",
ref_offset(raw));
- jffs2_mark_node_obsolete(c, raw);
+ /* prevent make deletion dirent obsolete directly*/
+ if (ref_flags(raw) != REF_NORMAL)
+ jffs2_mark_node_obsolete(c, raw);
raw = next;
}
diff --git a/fs.c b/fs.c
index 601afd1..ac3abe0 100644
--- a/fs.c
+++ b/fs.c
@@ -657,6 +657,7 @@ struct jffs2_inode_info *jffs2_gc_fetch_inode(struct
jffs2_sb_info *c,
ic->ino, ic->state);
sleep_on_spinunlock(&c->inocache_wq,
&c->inocache_lock);
} else {
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
}
diff --git a/gc.c b/gc.c
index 5a2dec2..3a0f84a 100644
--- a/gc.c
+++ b/gc.c
@@ -39,6 +39,14 @@ static int jffs2_garbage_collect_dnode(struct
jffs2_sb_info *c, struct jffs2_era
uint32_t start, uint32_t end);
static int jffs2_garbage_collect_live(struct jffs2_sb_info *c, struct
jffs2_eraseblock *jeb,
struct jffs2_raw_node_ref *raw, struct
jffs2_inode_info *f);
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info
*c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd);
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref
*raw_suspect,
+ struct jffs2_inode_cache *ic);
/* Called with erase_completion_lock held */
static struct jffs2_eraseblock *jffs2_find_gc_block(struct jffs2_sb_info
*c)
@@ -169,6 +177,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
if (!ic->pino_nlink) {
jffs2_dbg(1, "Skipping check of ino #%d with
nlink/pino zero\n",
ic->ino);
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
jffs2_xattr_delete_inode(c, ic);
continue;
@@ -358,9 +367,14 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
case INO_STATE_PRESENT:
/* It's in-core. GC must iget() it. */
+ case INO_STATE_DELETION:
+ /* Unlinked node. GC must check deletion dirent. */
break;
case INO_STATE_UNCHECKED:
+ ic->state = INO_STATE_DELETION;
+ break;
+
case INO_STATE_CHECKING:
case INO_STATE_GC:
/* Should never happen. We should have finished checking
@@ -431,19 +445,26 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info
*c)
nlink = ic->pino_nlink;
spin_unlock(&c->inocache_lock);
- f = jffs2_gc_fetch_inode(c, inum, !nlink);
+ if (ic->state == INO_STATE_DELETION)
+ f = NULL;
+ else
+ f = jffs2_gc_fetch_inode(c, inum, !nlink);
+
if (IS_ERR(f)) {
ret = PTR_ERR(f);
goto release_sem;
}
- if (!f) {
+ if (!f && ic->state != INO_STATE_DELETION) {
ret = 0;
goto release_sem;
}
- ret = jffs2_garbage_collect_live(c, jeb, raw, f);
-
- jffs2_gc_release_inode(c, f);
+ if (ic->state == INO_STATE_DELETION) {
+ ret = jffs2_garbage_collect_unlinked_dirent(c, jeb, raw,
ic);
+ } else {
+ ret = jffs2_garbage_collect_live(c, jeb, raw, f);
+ jffs2_gc_release_inode(c, f);
+ }
test_gcnode:
if (jeb->dirty_size == gcblock_dirty &&
!ref_obsolete(jeb->gc_node)) {
@@ -990,6 +1011,236 @@ static int
jffs2_garbage_collect_deletion_dirent(struct jffs2_sb_info *c, struct
return 0;
}
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info
*c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd)
+{
+ size_t retlen;
+ int ret;
+ uint32_t phys_ofs, alloclen;
+ uint32_t rawlen;
+ int retried = 0;
+
+ jffs2_dbg(1, "Going to GC REF_DELETION node at 0x%08x\n",
+ ref_offset(raw));
+ alloclen = rawlen = ref_totlen(c, c->gcblock, raw);
+
+ /* Ask for a small amount of space (or the totlen if smaller)
because we
+ don't want to force wastage of the end of a block if splitting
would
+ work. */
+ if (ic && alloclen > sizeof(struct jffs2_raw_inode) +
JFFS2_MIN_DATA_LEN)
+ alloclen = sizeof(struct jffs2_raw_inode) +
JFFS2_MIN_DATA_LEN;
+
+ ret = jffs2_reserve_space_gc(c, alloclen, &alloclen, rawlen);
+
+ if (ret)
+ goto out_node;
+
+ if (alloclen < rawlen) {
+ ret = -EBADFD;
+ goto out_node;
+ }
+
+ retry:
+ phys_ofs = write_ofs(c);
+
+ ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)rd);
+
+ if (ret || (retlen != rawlen)) {
+ pr_notice("Write of %d bytes at 0x%08x failed. returned
%d, retlen %zd\n",
+ rawlen, phys_ofs, ret, retlen);
+ if (retlen) {
+ jffs2_add_physical_node_ref(c, phys_ofs |
REF_OBSOLETE, rawlen, NULL);
+ } else {
+ pr_notice("Not marking the space at 0x%08x as
dirty because the flash driver returned retlen zero\n",
+ phys_ofs);
+ }
+ if (!retried) {
+ /* Try to reallocate space and retry */
+ uint32_t dummy;
+ struct jffs2_eraseblock *jeb = &c->blocks[phys_ofs
/ c->sector_size];
+
+ retried = 1;
+
+ jffs2_dbg(1, "Retrying failed write of
REF_DELETION node.\n");
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ ret = jffs2_reserve_space_gc(c, rawlen, &dummy,
rawlen);
+
+ if (!ret) {
+ jffs2_dbg(1, "Allocated space at 0x%08x to
retry failed write.\n",
+ phys_ofs);
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ goto retry;
+ }
+ jffs2_dbg(1, "Failed to allocate space to retry
failed write: %d!\n",
+ ret);
+ }
+
+ if (!ret)
+ ret = -EIO;
+ goto out_node;
+ }
+ jffs2_add_physical_node_ref(c, phys_ofs | REF_NORMAL, rawlen, ic);
+
+ jffs2_mark_node_obsolete(c, raw);
+ jffs2_dbg(1, "WHEEE! GC REF_DELETION node at 0x%08x succeeded\n",
+ ref_offset(raw));
+ out_node:
+ kfree(rd);
+ return ret;
+}
+
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref *raw_suspect,
+ struct jffs2_inode_cache *ic)
+{
+ struct jffs2_full_dirent *fd = NULL;
+ struct jffs2_raw_dirent *rd = NULL;
+ struct jffs2_raw_dirent *rd_suspect = NULL;
+
+ if (!jffs2_can_mark_obsolete(c)) {
+ int ret;
+ int name_len;
+ size_t retlen;
+ uint32_t checkedlen;
+ uint32_t name_crc;
+ uint32_t raw_len;
+ struct jffs2_raw_node_ref *raw;
+
+ raw_len = ref_totlen(c, jeb, raw_suspect);
+ rd_suspect = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd_suspect)
+ return -ENOMEM;
+
+ ret = jffs2_flash_read(c, ref_offset(raw_suspect),
+ raw_len, &retlen, (char *)rd_suspect);
+
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading deletion
dirent node at %08x\n",
+ __func__, ret, ref_offset(raw_suspect));
+ kfree(rd_suspect);
+ return -EIO;
+ }
+
+ if (je16_to_cpu(rd_suspect->nodetype) !=
JFFS2_NODETYPE_DIRENT) {
+ pr_warn("%s(): ofs(%08x) nodetype(%04x) is not
expected\n",
+ __func__, ref_offset(raw_suspect),
+ je16_to_cpu(rd_suspect->nodetype));
+ kfree(rd_suspect);
+ BUG();
+ }
+
+ if (je32_to_cpu(rd_suspect->ino)) {
+ jffs2_dbg(1, "%s(): ino %d not deletion dirent\n",
+ __func__, je32_to_cpu(rd_suspect->ino));
+ goto obsolete;
+ }
+
+ checkedlen = strnlen(rd_suspect->name, rd_suspect->nsize);
+ fd = jffs2_alloc_full_dirent(checkedlen+1);
+ if (!fd) {
+ kfree(rd_suspect);
+ return -ENOMEM;
+ }
+
+ memcpy(&fd->name, rd_suspect->name, checkedlen);
+ fd->name[checkedlen] = 0;
+ fd->raw = raw_suspect;
+ name_len = strlen(fd->name);
+ name_crc = crc32(0, fd->name, name_len);
+ rd = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd) {
+ kfree(rd_suspect);
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+
+ /* Prevent the erase code from nicking the obsolete node
refs
+ while we're looking at them. I really don't like this
extra lock but
+ can't see any alternative. Suggestions on a postcard to...
*/
+ mutex_lock(&c->erase_free_sem);
+ for (raw = ic->nodes; raw != (void *)ic; raw =
raw->next_in_ino) {
+
+ cond_resched();
+
+ /* We only care about obsolete ones */
+ if (!(ref_obsolete(raw)))
+ continue;
+
+ /* Any dirent with the same name is going to have
the same length... */
+ if (ref_totlen(c, NULL, raw) != raw_len)
+ continue;
+
+ /* Doesn't matter if there's one in the same erase
block. We're going to
+ delete it too at the same time. */
+ if (SECTOR_ADDR(raw->flash_offset) ==
SECTOR_ADDR(fd->raw->flash_offset))
+ continue;
+
+ jffs2_dbg(1, "Check potential deletion dirent at
%08x\n",
+ ref_offset(raw));
+
+ /* This is an obsolete node belonging to the same
directory, and it's of the right
+ length. We need to take a closer look...*/
+ ret = jffs2_flash_read(c, ref_offset(raw),
raw_len, &retlen, (char *)rd);
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading
obsolete node at %08x\n",
+ __func__, ret, ref_offset(raw));
+ /* If we can't read it, we don't need to
continue to obsolete it. Continue */
+ continue;
+ }
+ if (retlen != raw_len) {
+ pr_warn("%s(): Short read (%zd not %u)
reading header from obsolete node at %08x\n",
+ __func__, retlen, raw_len,
+ ref_offset(raw));
+ continue;
+ }
+
+ if (je16_to_cpu(rd->nodetype) !=
JFFS2_NODETYPE_DIRENT)
+ continue;
+
+ /* If the name CRC doesn't match, skip */
+ if (je32_to_cpu(rd->name_crc) != name_crc)
+ continue;
+
+ /* If the name length doesn't match, or it's
another deletion dirent, skip */
+ if (rd->nsize != name_len ||
!je32_to_cpu(rd->ino))
+ continue;
+
+ /* OK, check the actual name now */
+ if (memcmp(rd->name, fd->name, name_len))
+ continue;
+
+ /* OK. The name really does match. There really is
still an older node on
+ the flash which our deletion dirent obsoletes.
So we have to write out
+ a new deletion dirent to replace it */
+ mutex_unlock(&c->erase_free_sem);
+
+ jffs2_dbg(1, "Deletion dirent at %08x still
obsoletes real dirent \"%s\" at %08x for ino #%u\n",
+ ref_offset(fd->raw), fd->name,
+ ref_offset(raw), je32_to_cpu(rd->ino));
+ kfree(rd);
+ jffs2_free_full_dirent(fd);
+ return jffs2_garbage_collect_unlinked_deletion(c,
ic, raw_suspect, rd_suspect);
+ }
+ mutex_unlock(&c->erase_free_sem);
+ }
+obsolete:
+ jffs2_mark_node_obsolete(c, raw_suspect);
+ if (fd)
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ kfree(rd_suspect);
+ return 0;
+}
+
static int jffs2_garbage_collect_hole(struct jffs2_sb_info *c, struct
jffs2_eraseblock *jeb,
struct jffs2_inode_info *f, struct
jffs2_full_dnode *fn,
uint32_t start, uint32_t end)
diff --git a/nodelist.h b/nodelist.h
index fa35ff7..e8e282a 100644
--- a/nodelist.h
+++ b/nodelist.h
@@ -192,6 +192,7 @@ struct jffs2_inode_cache {
#define INO_STATE_GC 4 /* GCing a 'pristine' node */
#define INO_STATE_READING 5 /* In read_inode() */
#define INO_STATE_CLEARING 6 /* In clear_inode() */
+#define INO_STATE_DELETION 7 /* Unlinked inode */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate
xattr_ref */
diff --git a/summary.c b/summary.c
index c522d09..2488d50 100644
--- a/summary.c
+++ b/summary.c
@@ -470,17 +470,85 @@ static int jffs2_sum_process_sum_data(struct
jffs2_sb_info *c, struct jffs2_eras
return -ENOMEM;
}
- fd->raw = sum_link_node_ref(c, jeb,
je32_to_cpu(spd->offset) | REF_UNCHECKED,
- PAD(je32_to_cpu(spd->totlen)), ic);
-
+ fd->ino = je32_to_cpu(spd->ino);
+ if (fd->ino != 0) {
+ fd->raw = sum_link_node_ref(c,
jeb, je32_to_cpu(spd->offset) | REF_UNCHECKED,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ } else {
+ int ret;
+ size_t retlen;
+ uint32_t hdr_crc, crc;
+ struct jffs2_raw_dirent *rd;
+ uint32_t rd_totlen =
je32_to_cpu(spd->totlen);
+
+ rd = kmalloc(rd_totlen,
GFP_KERNEL);
+ if (!rd) {
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+ ret = jffs2_flash_read(c,
jeb->offset + je32_to_cpu(spd->offset),
+ rd_totlen, &retlen, (char
*)rd);
+ if (!ret && retlen != rd_totlen) {
+ pr_err("Dirent at %08x has
read problem. Aborting mount.\n",
+ jeb->offset +
je32_to_cpu(spd->offset));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ return -EIO;
+ }
+ hdr_crc = crc32(0, rd,
sizeof(struct jffs2_unknown_node)-4);
+ if (hdr_crc !=
je32_to_cpu(rd->hdr_crc)) {
+ pr_notice("%s(): Node at
0x%08x {0x%04x, 0x%04x, 0x%08x) has invalid CRC 0x%08x (calculated
0x%08x)\n",
+ __func__,
+ jeb->offset +
je32_to_cpu(spd->offset),
+ je16_to_cpu(rd->magic),
+ je16_to_cpu(rd->nodetype),
+ je32_to_cpu(rd->totlen),
+ je32_to_cpu(rd->hdr_crc),
+ hdr_crc);
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, rd, sizeof(*rd)-8);
+ if (crc !=
je32_to_cpu(rd->node_crc)) {
+ pr_notice("%s(): Node CRC
failed on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
+ __func__,
jeb->offset + je32_to_cpu(spd->offset),
+ je32_to_cpu(rd->node_crc), crc);
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, fd->name,
rd->nsize);
+ if (crc !=
je32_to_cpu(rd->name_crc)) {
+ pr_notice("%s(): Name CRC
failed on node at 0x%08x: Read 0x%08x, calculated 0x%08x\n",
+ __func__,
jeb->offset + je32_to_cpu(spd->offset),
+ je32_to_cpu(rd->name_crc), crc);
+ jffs2_dbg(1, "Name for
which CRC failed is (now) '%s', ino #%d\n",
+ fd->name,
je32_to_cpu(rd->ino));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err =
jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ kfree(rd);
+ fd->raw = sum_link_node_ref(c,
jeb, je32_to_cpu(spd->offset) | REF_NORMAL,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ }
fd->next = NULL;
fd->version = je32_to_cpu(spd->version);
- fd->ino = je32_to_cpu(spd->ino);
fd->nhash = full_name_hash(fd->name,
checkedlen);
fd->type = spd->type;
jffs2_add_fd_to_list(c, fd,
&ic->scan_dents);
-
+out:
*pseudo_random +=
je32_to_cpu(spd->version);
sp +=
JFFS2_SUMMARY_DIRENT_SIZE(spd->nsize);
diff --git a/write.c b/write.c
index b634de4..750257a 100644
--- a/write.c
+++ b/write.c
@@ -648,7 +648,7 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct
jffs2_inode_info *dir_f,
fd->name,
dead_f->inocache->ino);
}
- if (fd->raw)
+ if (fd->raw && fd->ino)
jffs2_mark_node_obsolete(c,
fd->raw);
jffs2_free_full_dirent(fd);
}
Signed-off-by: Liu Song <liu.song11@zte.com.cn>
--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-11-05 11:45 ` Brian Norris
2014-11-11 7:12 ` liu.song11
@ 2014-11-12 6:45 ` Liu Song
2014-12-17 1:53 ` Brian Norris
1 sibling, 1 reply; 13+ messages in thread
From: Liu Song @ 2014-11-12 6:45 UTC (permalink / raw)
To: Brian Norris
Cc: cui.yunfeng, linux-mtd, jiang.xuexin, wang.bo116, dwmw2,
deng.chao1
Brian Norris <computersforpeace@gmail.com> wrote on 2014-11-05 19:45:00:
> From: Brian Norris <computersforpeace@gmail.com>
> To: liu.song11@zte.com.cn,
> Cc: jiang.xuexin@zte.com.cn, linux-mtd@lists.infradead.org, cui.yunfeng@zte.com.cn, wang.bo116@zte.com.cn, dwmw2@infradead.org, deng.chao1@zte.com.cn
> Date: 2014-11-05 19:45
> Subject: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
>
> On Fri, Oct 24, 2014 at 05:26:05PM +0800, liu.song11@zte.com.cn wrote:
> > hi everyone
>
> Hi!
>
> > When jffs2 with summary, during the mounting period, in
> > jffs2_build_remove_unlinked_inode will
> > handle unlinke inode, but this will cause unlinked directory's deletion
> > dirent erased before
> > it's obsoleted dirent.So if the directory has renamed, then could cause a
> > hard link when we
> > remount jffs2.
> > We can stable reproduce the hard link only by following process.
> > 1. we mount jffs2 in /mnt
> > 2. create directories /mnt/SW1/, /mnt/SW2/
> > 3. create directory /mnt/old/
> > 4. rename /mnt/SW1/ to /mnt/old/SW1/, /mnt/SW2/ to /mnt/old/SW2/
> > 5. do some data write. don't touch these directories.
> > 6. rename /mnt/old/SW1/ to /mnt/SW1/, /mnt/old/SW2/ to /mnt/SW2/
> > 7. delete /mnt/old/
> >
> > do above process 3 - 7 several times, then reboot, during the mounting
> > process, we could found
> > /mnt/SW1/ and /mnt/old/SW1/ become hard link,/mnt/SW2/ and /mnt/old/SW2/
> > become hard link. So we
> > do some job to fix this bug.We have tested this patch and the hard link
> > error will never occur.
> > Thanks!
> > Best Regards
>
> This only describes the problem, but it doesn't describe the fix at all.
> Please provide a proper commit description.
>
> Also, your patch is very big and line-wrapped, so it's very hard to
> figure out what it's doing, and it won't apply to my git tree. Please
> check your mailer settings.
>
> All this is addressed in Documentation/SubmittingPatches, which I
> suggest you read.
>
> Regards,
> Brian
Hi, Brian
Thanks a lot for your attention and suggestions. Follow the guide, patch had checked by checkpatch.pl,
but there also had warnings about line over 80 characters, I had no idea to fix that except remove the
code indentations, but i'm afraid that would make codes hard to read. However, if there are any
suggestions to fix this I would try. Patch has really long print notice in summary.c, we remove them
from this patch.
The following is the description about this patch.
The patch is focus on jffs2 with summary. We check deletion dirent's crc in "jffs2_sum_process_sum_data"
so could make its ref flag REF_NORMAL. After this process, all crc correct deletion dirents' ref flag
are REF_NORMAL then could separate from other nodes(REF_UNCHECKED).We use this flag to protect unlinked
deletion dirent directly turned to obsoleted in "jffs2_build_remove_unlinked_inode", because deletion dirents
maybe still useful if it's obsoleted dirent remain in flash. These unlinked deletion dirents will be checked
whether still useful in gc time.
These unlinked deletion dirents in "jffs2_garbage_collect_pass" will skip to check, because its
"jffs2_inode_cache"'s pino_nlink is zero. Then in "jffs2_garbage_collect_pass", unlinked deletion dirents' "ic->state"
is INO_STATE_UNCHECKED, but we had checked the crc during scan. So we give its a new state which is INO_STATE_DELETION.
If gc find current node's "ic->state" is INO_STATE_DELETION will know it's an unlinked deletion dirent, then will check
whether its still useful in "jffs2_garbage_collect_unlinked_dirent" which is similar to "jffs2_garbage_collect_deletion_dirent"
but use different parameters. If the unlinked deletion dirent is still useful, we copy it, else, we could safely mark it obsoleted.
In "jffs2_do_unlink", there also has the same problem, it will obsolete the unlinked directory's child dirents
which include deletion dirent, so we should also protect the deletion dirent.
The main purpose of this patch is to prevent unlinked deletion direct directly turn to obsolete and handle it
during gc time. We had do many tests on this patch and it worked properly, the directory hard link couldn't occured.
We had also test the deletion dirents' crc in scan time whether cost a lot,the result is check 81809 deletion dirents' crc
could within one second, so it will not extend the mounting time.
Thanks
Best Regards
Liu Song
-------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/build.c b/build.c
index a3750f9..ad7b8a6 100644
--- a/build.c
+++ b/build.c
@@ -205,7 +205,9 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
while (raw != (void *)ic) {
struct jffs2_raw_node_ref *next = raw->next_in_ino;
dbg_fsbuild("obsoleting node at 0x%08x\n", ref_offset(raw));
- jffs2_mark_node_obsolete(c, raw);
+ /* prevent make deletion dirent obsolete directly*/
+ if (ref_flags(raw) != REF_NORMAL)
+ jffs2_mark_node_obsolete(c, raw);
raw = next;
}
diff --git a/fs.c b/fs.c
index 601afd1..ac3abe0 100644
--- a/fs.c
+++ b/fs.c
@@ -657,6 +657,7 @@ struct jffs2_inode_info *jffs2_gc_fetch_inode(struct jffs2_sb_info *c,
ic->ino, ic->state);
sleep_on_spinunlock(&c->inocache_wq, &c->inocache_lock);
} else {
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
}
diff --git a/gc.c b/gc.c
index 5a2dec2..2b931f7 100644
--- a/gc.c
+++ b/gc.c
@@ -39,6 +39,14 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
uint32_t start, uint32_t end);
static int jffs2_garbage_collect_live(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
struct jffs2_raw_node_ref *raw, struct jffs2_inode_info *f);
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info *c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd);
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref *raw_suspect,
+ struct jffs2_inode_cache *ic);
/* Called with erase_completion_lock held */
static struct jffs2_eraseblock *jffs2_find_gc_block(struct jffs2_sb_info *c)
@@ -169,6 +177,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
if (!ic->pino_nlink) {
jffs2_dbg(1, "Skipping check of ino #%d with nlink/pino zero\n",
ic->ino);
+ ic->state = INO_STATE_DELETION;
spin_unlock(&c->inocache_lock);
jffs2_xattr_delete_inode(c, ic);
continue;
@@ -358,9 +367,14 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
case INO_STATE_PRESENT:
/* It's in-core. GC must iget() it. */
+ case INO_STATE_DELETION:
+ /* Unlinked node. GC must check deletion dirent. */
break;
case INO_STATE_UNCHECKED:
+ ic->state = INO_STATE_DELETION;
+ break;
+
case INO_STATE_CHECKING:
case INO_STATE_GC:
/* Should never happen. We should have finished checking
@@ -431,19 +445,26 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
nlink = ic->pino_nlink;
spin_unlock(&c->inocache_lock);
- f = jffs2_gc_fetch_inode(c, inum, !nlink);
+ if (ic->state == INO_STATE_DELETION)
+ f = NULL;
+ else
+ f = jffs2_gc_fetch_inode(c, inum, !nlink);
+
if (IS_ERR(f)) {
ret = PTR_ERR(f);
goto release_sem;
}
- if (!f) {
+ if (!f && ic->state != INO_STATE_DELETION) {
ret = 0;
goto release_sem;
}
- ret = jffs2_garbage_collect_live(c, jeb, raw, f);
-
- jffs2_gc_release_inode(c, f);
+ if (ic->state == INO_STATE_DELETION) {
+ ret = jffs2_garbage_collect_unlinked_dirent(c, jeb, raw, ic);
+ } else {
+ ret = jffs2_garbage_collect_live(c, jeb, raw, f);
+ jffs2_gc_release_inode(c, f);
+ }
test_gcnode:
if (jeb->dirty_size == gcblock_dirty && !ref_obsolete(jeb->gc_node)) {
@@ -990,6 +1011,235 @@ static int jffs2_garbage_collect_deletion_dirent(struct jffs2_sb_info *c, struct
return 0;
}
+static int jffs2_garbage_collect_unlinked_deletion(struct jffs2_sb_info *c,
+ struct jffs2_inode_cache *ic,
+ struct jffs2_raw_node_ref *raw,
+ struct jffs2_raw_dirent *rd)
+{
+ size_t retlen;
+ int ret;
+ uint32_t phys_ofs, alloclen;
+ uint32_t rawlen;
+ int retried = 0;
+
+ jffs2_dbg(1, "Going to GC REF_DELETION node at 0x%08x\n",
+ ref_offset(raw));
+ alloclen = rawlen = ref_totlen(c, c->gcblock, raw);
+
+ /* Ask for a small amount of space (or the totlen if smaller) because we
+ don't want to force wastage of the end of a block if splitting would
+ work. */
+ if (ic && alloclen > sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN)
+ alloclen = sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN;
+
+ ret = jffs2_reserve_space_gc(c, alloclen, &alloclen, rawlen);
+
+ if (ret)
+ goto out_node;
+
+ if (alloclen < rawlen) {
+ ret = -EBADFD;
+ goto out_node;
+ }
+
+ retry:
+ phys_ofs = write_ofs(c);
+
+ ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)rd);
+
+ if (ret || (retlen != rawlen)) {
+ pr_notice("Write of %d bytes at 0x%08x failed. returned %d, retlen %zd\n",
+ rawlen, phys_ofs, ret, retlen);
+ if (retlen)
+ jffs2_add_physical_node_ref(c, phys_ofs | REF_OBSOLETE, rawlen, NULL);
+ else
+ pr_notice("Not marking the space at 0x%08x as dirty because retlen zero\n",
+ phys_ofs);
+ if (!retried) {
+ /* Try to reallocate space and retry */
+ uint32_t dummy;
+ struct jffs2_eraseblock *jeb = &c->blocks[phys_ofs / c->sector_size];
+
+ retried = 1;
+
+ jffs2_dbg(1, "Retrying failed write of REF_DELETION node.\n");
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ ret = jffs2_reserve_space_gc(c, rawlen, &dummy, rawlen);
+
+ if (!ret) {
+ jffs2_dbg(1, "Allocated space at 0x%08x to retry failed write.\n",
+ phys_ofs);
+
+ jffs2_dbg_acct_sanity_check(c, jeb);
+ jffs2_dbg_acct_paranoia_check(c, jeb);
+
+ goto retry;
+ }
+ jffs2_dbg(1, "Failed to allocate space to retry failed write: %d!\n",
+ ret);
+ }
+
+ if (!ret)
+ ret = -EIO;
+ goto out_node;
+ }
+ jffs2_add_physical_node_ref(c, phys_ofs | REF_NORMAL, rawlen, ic);
+
+ jffs2_mark_node_obsolete(c, raw);
+ jffs2_dbg(1, "WHEEE! GC REF_DELETION node at 0x%08x succeeded\n",
+ ref_offset(raw));
+ out_node:
+ kfree(rd);
+ return ret;
+}
+
+static int jffs2_garbage_collect_unlinked_dirent(struct jffs2_sb_info *c,
+ struct jffs2_eraseblock *jeb,
+ struct jffs2_raw_node_ref *raw_suspect,
+ struct jffs2_inode_cache *ic)
+{
+ struct jffs2_full_dirent *fd = NULL;
+ struct jffs2_raw_dirent *rd = NULL;
+ struct jffs2_raw_dirent *rd_suspect = NULL;
+
+ if (!jffs2_can_mark_obsolete(c)) {
+ int ret;
+ int name_len;
+ size_t retlen;
+ uint32_t checkedlen;
+ uint32_t name_crc;
+ uint32_t raw_len;
+ struct jffs2_raw_node_ref *raw;
+
+ raw_len = ref_totlen(c, jeb, raw_suspect);
+ rd_suspect = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd_suspect)
+ return -ENOMEM;
+
+ ret = jffs2_flash_read(c, ref_offset(raw_suspect),
+ raw_len, &retlen, (char *)rd_suspect);
+
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading deletion dirent node at %08x\n",
+ __func__, ret, ref_offset(raw_suspect));
+ kfree(rd_suspect);
+ return -EIO;
+ }
+
+ if (je16_to_cpu(rd_suspect->nodetype) != JFFS2_NODETYPE_DIRENT) {
+ pr_warn("%s(): ofs(%08x) nodetype(%04x) is not expected\n",
+ __func__, ref_offset(raw_suspect),
+ je16_to_cpu(rd_suspect->nodetype));
+ kfree(rd_suspect);
+ BUG();
+ }
+
+ if (je32_to_cpu(rd_suspect->ino)) {
+ jffs2_dbg(1, "%s(): ino %d not deletion dirent\n",
+ __func__, je32_to_cpu(rd_suspect->ino));
+ goto obsolete;
+ }
+
+ checkedlen = strnlen(rd_suspect->name, rd_suspect->nsize);
+ fd = jffs2_alloc_full_dirent(checkedlen+1);
+ if (!fd) {
+ kfree(rd_suspect);
+ return -ENOMEM;
+ }
+
+ memcpy(&fd->name, rd_suspect->name, checkedlen);
+ fd->name[checkedlen] = 0;
+ fd->raw = raw_suspect;
+ name_len = strlen(fd->name);
+ name_crc = crc32(0, fd->name, name_len);
+ rd = kmalloc(raw_len, GFP_KERNEL);
+ if (!rd) {
+ kfree(rd_suspect);
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+
+ /* Prevent the erase code from nicking the obsolete node refs
+ while we're looking at them. I really don't like this extra lock but
+ can't see any alternative. Suggestions on a postcard to... */
+ mutex_lock(&c->erase_free_sem);
+ for (raw = ic->nodes; raw != (void *)ic; raw = raw->next_in_ino) {
+
+ cond_resched();
+
+ /* We only care about obsolete ones */
+ if (!(ref_obsolete(raw)))
+ continue;
+
+ /* Any dirent with the same name is going to have the same length... */
+ if (ref_totlen(c, NULL, raw) != raw_len)
+ continue;
+
+ /* Doesn't matter if there's one in the same erase block. We're going to
+ delete it too at the same time. */
+ if (SECTOR_ADDR(raw->flash_offset) == SECTOR_ADDR(fd->raw->flash_offset))
+ continue;
+
+ jffs2_dbg(1, "Check potential deletion dirent at %08x\n",
+ ref_offset(raw));
+
+ /* This is an obsolete node belonging to the same directory, and it's of the right
+ length. We need to take a closer look...*/
+ ret = jffs2_flash_read(c, ref_offset(raw), raw_len, &retlen, (char *)rd);
+ if (ret) {
+ pr_warn("%s(): Read error (%d) reading obsolete node at %08x\n",
+ __func__, ret, ref_offset(raw));
+ /* If we can't read it, we don't need to continue to obsolete it. Continue */
+ continue;
+ }
+ if (retlen != raw_len) {
+ pr_warn("%s(): Short read (%zd not %u) reading header from obsolete node at %08x\n",
+ __func__, retlen, raw_len,
+ ref_offset(raw));
+ continue;
+ }
+
+ if (je16_to_cpu(rd->nodetype) != JFFS2_NODETYPE_DIRENT)
+ continue;
+
+ /* If the name CRC doesn't match, skip */
+ if (je32_to_cpu(rd->name_crc) != name_crc)
+ continue;
+
+ /* If the name length doesn't match, or it's another deletion dirent, skip */
+ if (rd->nsize != name_len || !je32_to_cpu(rd->ino))
+ continue;
+
+ /* OK, check the actual name now */
+ if (memcmp(rd->name, fd->name, name_len))
+ continue;
+
+ /* OK. The name really does match. There really is still an older node on
+ the flash which our deletion dirent obsoletes. So we have to write out
+ a new deletion dirent to replace it */
+ mutex_unlock(&c->erase_free_sem);
+
+ jffs2_dbg(1, "Deletion dirent at %08x still obsoletes real dirent \"%s\" at %08x for ino #%u\n",
+ ref_offset(fd->raw), fd->name,
+ ref_offset(raw), je32_to_cpu(rd->ino));
+ kfree(rd);
+ jffs2_free_full_dirent(fd);
+ return jffs2_garbage_collect_unlinked_deletion(c, ic, raw_suspect, rd_suspect);
+ }
+ mutex_unlock(&c->erase_free_sem);
+ }
+obsolete:
+ jffs2_mark_node_obsolete(c, raw_suspect);
+ if (fd)
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ kfree(rd_suspect);
+ return 0;
+}
+
static int jffs2_garbage_collect_hole(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
struct jffs2_inode_info *f, struct jffs2_full_dnode *fn,
uint32_t start, uint32_t end)
diff --git a/nodelist.h b/nodelist.h
index fa35ff7..e8e282a 100644
--- a/nodelist.h
+++ b/nodelist.h
@@ -192,6 +192,7 @@ struct jffs2_inode_cache {
#define INO_STATE_GC 4 /* GCing a 'pristine' node */
#define INO_STATE_READING 5 /* In read_inode() */
#define INO_STATE_CLEARING 6 /* In clear_inode() */
+#define INO_STATE_DELETION 7 /* Unlinked inode */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
diff --git a/summary.c b/summary.c
index c522d09..fd58537 100644
--- a/summary.c
+++ b/summary.c
@@ -470,17 +470,73 @@ static int jffs2_sum_process_sum_data(struct jffs2_sb_info *c, struct jffs2_eras
return -ENOMEM;
}
- fd->raw = sum_link_node_ref(c, jeb, je32_to_cpu(spd->offset) | REF_UNCHECKED,
- PAD(je32_to_cpu(spd->totlen)), ic);
-
+ fd->ino = je32_to_cpu(spd->ino);
+ if (fd->ino != 0) {
+ fd->raw = sum_link_node_ref(c, jeb,
+ je32_to_cpu(spd->offset) | REF_UNCHECKED,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ } else {
+ int ret;
+ size_t retlen;
+ uint32_t hdr_crc, crc;
+ struct jffs2_raw_dirent *rd;
+ uint32_t rd_totlen = je32_to_cpu(spd->totlen);
+
+ rd = kmalloc(rd_totlen, GFP_KERNEL);
+ if (!rd) {
+ jffs2_free_full_dirent(fd);
+ return -ENOMEM;
+ }
+ ret = jffs2_flash_read(c, jeb->offset + je32_to_cpu(spd->offset),
+ rd_totlen, &retlen, (char *)rd);
+ if (!ret && retlen != rd_totlen) {
+ pr_err("Dirent at %08x has read problem. Aborting mount.\n",
+ jeb->offset + je32_to_cpu(spd->offset));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ return -EIO;
+ }
+ hdr_crc = crc32(0, rd, sizeof(struct jffs2_unknown_node)-4);
+ if (hdr_crc != je32_to_cpu(rd->hdr_crc)) {
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err = jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, rd, sizeof(*rd)-8);
+ if (crc != je32_to_cpu(rd->node_crc)) {
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err = jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ crc = crc32(0, fd->name, rd->nsize);
+ if (crc != je32_to_cpu(rd->name_crc)) {
+ jffs2_dbg(1, "Name for which CRC failed is (now) '%s', ino #%d\n",
+ fd->name, je32_to_cpu(rd->ino));
+ jffs2_free_full_dirent(fd);
+ kfree(rd);
+ err = jffs2_scan_dirty_space(c, jeb, PAD(rd_totlen));
+ if (err)
+ return err;
+ goto out;
+ }
+ kfree(rd);
+ fd->raw = sum_link_node_ref(c, jeb,
+ je32_to_cpu(spd->offset) | REF_NORMAL,
+ PAD(je32_to_cpu(spd->totlen)), ic);
+ }
fd->next = NULL;
fd->version = je32_to_cpu(spd->version);
- fd->ino = je32_to_cpu(spd->ino);
fd->nhash = full_name_hash(fd->name, checkedlen);
fd->type = spd->type;
jffs2_add_fd_to_list(c, fd, &ic->scan_dents);
-
+out:
*pseudo_random += je32_to_cpu(spd->version);
sp += JFFS2_SUMMARY_DIRENT_SIZE(spd->nsize);
diff --git a/write.c b/write.c
index b634de4..750257a 100644
--- a/write.c
+++ b/write.c
@@ -648,7 +648,7 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
fd->name,
dead_f->inocache->ino);
}
- if (fd->raw)
+ if (fd->raw && fd->ino)
jffs2_mark_node_obsolete(c, fd->raw);
jffs2_free_full_dirent(fd);
}
Signed-off-by: Liu Song <liu.song11@zte.com.cn>
--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-11-12 6:45 ` Liu Song
@ 2014-12-17 1:53 ` Brian Norris
2014-12-17 2:02 ` nick
0 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2014-12-17 1:53 UTC (permalink / raw)
To: Liu Song
Cc: cui.yunfeng, Artem Bityutskiy, linux-mtd, jiang.xuexin,
wang.bo116, dwmw2, deng.chao1
+ Artem
On Wed, Nov 12, 2014 at 02:45:09PM +0800, Liu Song wrote:
> Brian Norris <computersforpeace@gmail.com> wrote on 2014-11-05 19:45:00:
>
> > From: Brian Norris <computersforpeace@gmail.com>
> > To: liu.song11@zte.com.cn,
> > Cc: jiang.xuexin@zte.com.cn, linux-mtd@lists.infradead.org, cui.yunfeng@zte.com.cn, wang.bo116@zte.com.cn, dwmw2@infradead.org, deng.chao1@zte.com.cn
> > Date: 2014-11-05 19:45
> > Subject: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
> >
[...]
> > This only describes the problem, but it doesn't describe the fix at all.
> > Please provide a proper commit description.
> >
> > Also, your patch is very big and line-wrapped, so it's very hard to
> > figure out what it's doing, and it won't apply to my git tree. Please
> > check your mailer settings.
> >
> > All this is addressed in Documentation/SubmittingPatches, which I
> > suggest you read.
>
> Thanks a lot for your attention and suggestions. Follow the guide, patch had checked by checkpatch.pl,
> but there also had warnings about line over 80 characters, I had no idea to fix that except remove the
> code indentations, but i'm afraid that would make codes hard to read. However, if there are any
> suggestions to fix this I would try.
Sometimes you can break a line in the middle, where necessary. The C
language allows line-breaks in statements.
But many times, high levels of indentation mean you should probably
restructure your code. Documentation/CodingStyle talks about this.
> Patch has really long print notice in summary.c, we remove them
> from this patch.
>
> The following is the description about this patch.
OK, then format it as such. Try applying it with git-am, and you'll see
that it does not turn out well. See Documentation/email-clients.txt for
help on git.
> The patch is focus on jffs2 with summary. We check deletion dirent's crc in "jffs2_sum_process_sum_data"
> so could make its ref flag REF_NORMAL. After this process, all crc correct deletion dirents' ref flag
> are REF_NORMAL then could separate from other nodes(REF_UNCHECKED).We use this flag to protect unlinked
> deletion dirent directly turned to obsoleted in "jffs2_build_remove_unlinked_inode", because deletion dirents
> maybe still useful if it's obsoleted dirent remain in flash. These unlinked deletion dirents will be checked
> whether still useful in gc time.
>
> These unlinked deletion dirents in "jffs2_garbage_collect_pass" will skip to check, because its
> "jffs2_inode_cache"'s pino_nlink is zero. Then in "jffs2_garbage_collect_pass", unlinked deletion dirents' "ic->state"
> is INO_STATE_UNCHECKED, but we had checked the crc during scan. So we give its a new state which is INO_STATE_DELETION.
> If gc find current node's "ic->state" is INO_STATE_DELETION will know it's an unlinked deletion dirent, then will check
> whether its still useful in "jffs2_garbage_collect_unlinked_dirent" which is similar to "jffs2_garbage_collect_deletion_dirent"
> but use different parameters. If the unlinked deletion dirent is still useful, we copy it, else, we could safely mark it obsoleted.
>
> In "jffs2_do_unlink", there also has the same problem, it will obsolete the unlinked directory's child dirents
> which include deletion dirent, so we should also protect the deletion dirent.
>
> The main purpose of this patch is to prevent unlinked deletion direct directly turn to obsolete and handle it
> during gc time. We had do many tests on this patch and it worked properly, the directory hard link couldn't occured.
> We had also test the deletion dirents' crc in scan time whether cost a lot,the result is check 81809 deletion dirents' crc
> could within one second, so it will not extend the mounting time.
>
> Thanks
> Best Regards
> Liu Song
> -------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> diff --git a/build.c b/build.c
> index a3750f9..ad7b8a6 100644
> --- a/build.c
> +++ b/build.c
This means you're not patching at the top-level directory. It should say
a/fs/jffs2/build.c
(Same throughout this diff.)
Again, please test with git-am, and you'll see that this does not apply
well.
(snip rest of patch)
Honestly, you haven't done anything to make this patch more logically
easy to follow, and I'm not a JFFS2 developer. So there's really not any
way that I'm going to be able to handle this. And JFFS2 is past its
prime, with no one actually interested in supporting it.
Now, given this, there is a large burden on you to make it easy for
someone to accept your patch. It's not obvious you've done your homework
here, as the patch still doesn't apply right, and it is very hard to
read.
Technically, MAINTAINERS still lists David as the sole maintainer of
JFFS2. If he's not willing to review this patch, then perhaps JFFS2
should be marked orphan / obsolete:
diff --git a/MAINTAINERS b/MAINTAINERS
index a20df9bf8ab0..223889b30a51 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5198,10 +5198,9 @@ S: Maintained
F: drivers/net/ethernet/jme.*
JOURNALLING FLASH FILE SYSTEM V2 (JFFS2)
-M: David Woodhouse <dwmw2@infradead.org>
L: linux-mtd@lists.infradead.org
W: http://www.linux-mtd.infradead.org/doc/jffs2.html
-S: Maintained
+S: Orphan / Obsolete
F: fs/jffs2/
F: include/uapi/linux/jffs2.h
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-12-17 1:53 ` Brian Norris
@ 2014-12-17 2:02 ` nick
0 siblings, 0 replies; 13+ messages in thread
From: nick @ 2014-12-17 2:02 UTC (permalink / raw)
To: Brian Norris, Liu Song
Cc: cui.yunfeng, Artem Bityutskiy, linux-mtd, jiang.xuexin,
wang.bo116, dwmw2, deng.chao1
Greetings Brian,
Are you sure jffs2 is obsolete now? I seem to known of a few areas in embedded that are still using this file system unless we are moving toward f2fs for flash file systems. If that is the case why not just remove it from main line as
no one is supporting it outside legacy code for older devices and embedded systems.
Regards Nick
On 2014-12-16 08:53 PM, Brian Norris wrote:
>> > Thanks a lot for your attention and suggestions. Follow the guide, patch had checked by checkpatch.pl,
>> > but there also had warnings about line over 80 characters, I had no idea to fix that except remove the
>> > code indentations, but i'm afraid that would make codes hard to read. However, if there are any
>> > suggestions to fix this I would try.
> Sometimes you can break a line in the middle, where necessar
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2014-10-24 9:26 [PATCH]jffs2:bugfix for should not appeared directory hard link liu.song11
2014-11-05 11:45 ` Brian Norris
@ 2015-02-12 13:43 ` David Woodhouse
2015-02-13 4:09 ` liu.song11
1 sibling, 1 reply; 13+ messages in thread
From: David Woodhouse @ 2015-02-12 13:43 UTC (permalink / raw)
To: liu.song11; +Cc: wang.bo116, cui.yunfeng, linux-mtd, jiang.xuexin, deng.chao1
[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]
On Fri, 2014-10-24 at 17:26 +0800, liu.song11@zte.com.cn wrote:
>
> When jffs2 with summary, during the mounting period, in
> jffs2_build_remove_unlinked_inode will handle unlinke inode, but this
> will cause unlinked directory's deletion dirent erased before
> it's obsoleted dirent.So if the directory has renamed, then could
> cause a hard link when we remount jffs2.
> We can stable reproduce the hard link only by following process.
> 1. we mount jffs2 in /mnt
> 2. create directories /mnt/SW1/, /mnt/SW2/
> 3. create directory /mnt/old/
> 4. rename /mnt/SW1/ to /mnt/old/SW1/, /mnt/SW2/ to /mnt/old/SW2/
> 5. do some data write. don't touch these directories.
> 6. rename /mnt/old/SW1/ to /mnt/SW1/, /mnt/old/SW2/ to /mnt/SW2/
> 7. delete /mnt/old/
>
> do above process 3 - 7 several times, then reboot, during the mounting
> process, we could found
> /mnt/SW1/ and /mnt/old/SW1/ become hard link,/mnt/SW2/
> and /mnt/old/SW2/ become hard link. So we
> do some job to fix this bug.We have tested this patch and the hard
> link error will never occur.
Hi, apologies for the delay in looking at this.
I'm trying to understand the root cause of the problem. It looks like
the real problem here is that the /mnt/old/ directory seems to exist,
when it shouldn't?
It shouldn't *matter* that we erased the deletion dirents from /mnt/old/
leaving potentially *valid* dirents pointing to the "SW1" and "SW2"
inodes, should it? Because the /mnt/ directory *itself* shouldn't exist.
As far as I can tell, your patch seems to focus on the dirents pointing
to the SW1 and SW2 directories. But I think that's the wrong approach.
On mounting the file system again after /mnt/old was supposed to have
been deleted, *why* is it still appearing? Can you capture debug
messages over a serial console with CONFIG_JFFS2_FS_DEBUG=2?
I'd like to see the part in step #7 where it actually writes the
deletion dirent for /mnt/foo, followed by the full log of the remount
where it erroneously decides that /mnt/foo still exists.
Thanks.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2015-02-12 13:43 ` David Woodhouse
@ 2015-02-13 4:09 ` liu.song11
2015-02-13 10:13 ` David Woodhouse
0 siblings, 1 reply; 13+ messages in thread
From: liu.song11 @ 2015-02-13 4:09 UTC (permalink / raw)
To: David Woodhouse
Cc: jiang.xuexin, wang.bo116, linux-mtd, deng.chao1, cui.yunfeng
David Woodhouse <dwmw2@infradead.org> wrote on 2015-02-12 21:43:19:
> From: David Woodhouse <dwmw2@infradead.org>
> To: liu.song11@zte.com.cn,
> Cc: linux-mtd@lists.infradead.org, wang.bo116@zte.com.cn, deng.chao1@zte.com.cn, jiang.xuexin@zte.com.cn, cui.yunfeng@zte.com.cn
> Date: 2015-02-12 21:43
> Subject: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
>
> On Fri, 2014-10-24 at 17:26 +0800, liu.song11@zte.com.cn wrote:
> >
> > When jffs2 with summary, during the mounting period, in
> > jffs2_build_remove_unlinked_inode will handle unlinke inode, but this
> > will cause unlinked directory's deletion dirent erased before
> > it's obsoleted dirent.So if the directory has renamed, then could
> > cause a hard link when we remount jffs2.
>
> > We can stable reproduce the hard link only by following process.
> > 1. we mount jffs2 in /mnt
> > 2. create directories /mnt/SW1/, /mnt/SW2/
> > 3. create directory /mnt/old/
> > 4. rename /mnt/SW1/ to /mnt/old/SW1/, /mnt/SW2/ to /mnt/old/SW2/
> > 5. do some data write. don't touch these directories.
> > 6. rename /mnt/old/SW1/ to /mnt/SW1/, /mnt/old/SW2/ to /mnt/SW2/
> > 7. delete /mnt/old/
> >
> > do above process 3 - 7 several times, then reboot, during the mounting
> > process, we could found
> > /mnt/SW1/ and /mnt/old/SW1/ become hard link,/mnt/SW2/
> > and /mnt/old/SW2/ become hard link. So we
> > do some job to fix this bug.We have tested this patch and the hard
> > link error will never occur.
>
> Hi, apologies for the delay in looking at this.
>
> I'm trying to understand the root cause of the problem. It looks like
> the real problem here is that the /mnt/old/ directory seems to exist,
> when it shouldn't?
>
> It shouldn't *matter* that we erased the deletion dirents from /mnt/old/
> leaving potentially *valid* dirents pointing to the "SW1" and "SW2"
> inodes, should it? Because the /mnt/ directory *itself* shouldn't exist.
>
> As far as I can tell, your patch seems to focus on the dirents pointing
> to the SW1 and SW2 directories. But I think that's the wrong approach.
>
> On mounting the file system again after /mnt/old was supposed to have
> been deleted, *why* is it still appearing? Can you capture debug
> messages over a serial console with CONFIG_JFFS2_FS_DEBUG=2?
>
> I'd like to see the part in step #7 where it actually writes the
> deletion dirent for /mnt/foo, followed by the full log of the remount
> where it erroneously decides that /mnt/foo still exists.
>
> Thanks.
>
> --
> dwmw2
Hi David，
Thanks for your attention.
Here we use a jffs2 filesystem's image to describe this problem step by step.
1. we choose /mnt as the mount point of jffs2(mount -t jffs2 /dev/mtdblock0 /mnt)
mtdblock0 has just been formatted in jffs2 format, so there is nothing under /mnt after we mount it.
2. we create directory SW1, SW2, SW3, SW4, old under /mnt in sequence, now we have /mnt/SW1/, /mnt/SW2/,
/mnt/SW3/, /mnt/SW4/,/mnt/old/. We could know，/mnt/SW1's ino number is 2, /mnt/SW2's ino number is 3,
/mnt/SW3's ino number is 4,/mnt/SW4's ino number is 5.
3. mv /mnt/SW1 /mnt/old/SW1
mv /mnt/SW2 /mnt/old/SW2
mv /mnt/SW3 /mnt/old/SW3
mv /mnt/SW4 /mnt/old/SW4
4. mv /mnt/old/SW1 /mnt/SW1
mv /mnt/old/SW2 /mnt/SW2
mv /mnt/old/SW3 /mnt/SW3
mv /mnt/old/SW4 /mnt/SW4
5. rmdir /mnt/old
6. mkdir /mnt/old
7. goto step 3
In this loop, there only /mnt/old's ino number will changed.
Before the problem happen, here we print the dirents' info(after step 3 and before step4).
We can see，/mnt/old/SW1's pino is 71, that means /mnt/old's ino number is 71.
The dirents all in same eraseblock(0x0e600000 - 0x0e61FFFF).
-------------------------------------------------------------
ino = 2, name = SW1, version = 2, pino = 71, ofs = 0x0e6186e8
ino = 3, name = SW2, version = 3, pino = 71, ofs = 0x0e618740
ino = 4, name = SW3, version = 4, pino = 71, ofs = 0x0e618798
ino = 5, name = SW4, version = 5, pino = 71, ofs = 0x0e6187f0
ino = 71, name = old, version = 299, pino = 1, ofs = 0x0e6186bc
-------------------------------------------------------------
After step5 and before step6, we print the dirents' info.
We can see, SW1 - SW4 deletion dirents' all in same eraseblock(0x05680000 - 0x0569FFFF).
--------------------------------------------------------------
ino = 0, name = SW1, version = 6, pino = 71, ofs = 0x056803e4
ino = 0, name = SW2, version = 7, pino = 71, ofs = 0x0568043c
ino = 0, name = SW3, version = 8, pino = 71, ofs = 0x05680494
ino = 0, name = SW4, version = 9, pino = 71, ofs = 0x056804ec
ino = 2, name = SW1, version = 2, pino = 71, ofs = 0x0e6186e8
ino = 3, name = SW2, version = 3, pino = 71, ofs = 0x0e618740
ino = 4, name = SW3, version = 4, pino = 71, ofs = 0x0e618798
ino = 5, name = SW4, version = 5, pino = 71, ofs = 0x0e6187f0
ino = 0, name = old, version = 298, pino = 1, ofs = 0x0e6185c4
ino = 71, name = old, version = 299, pino = 1, ofs = 0x0e6186bc
---------------------------------------------------------------
Here, we stop and remount jffs2. Because ino = 0, name = old, version = 298, pino = 1, ofs = 0x0e6185c4 exist,
so /mnt/old will be handled in "jffs2_build_remove_unlinked_inode". In this function, /mnt/old/(ino:71) 's child dirent
will been mark obsolete.
--------------------------------------------------------------
ino = 0, name = SW1, version = 6, pino = 71, ofs = 0x056803e4 <---mark obsolete in jffs2_build_remove_unlinked_inode
ino = 0, name = SW2, version = 7, pino = 71, ofs = 0x0568043c <---mark obsolete in jffs2_build_remove_unlinked_inode
ino = 0, name = SW3, version = 8, pino = 71, ofs = 0x05680494 <---mark obsolete in jffs2_build_remove_unlinked_inode
ino = 0, name = SW4, version = 9, pino = 71, ofs = 0x056804ec <---mark obsolete in jffs2_build_remove_unlinked_inode
ino = 2, name = SW1, version = 2, pino = 71, ofs = 0x0e6186e8 <---mark obsolete by ino = 0, name = SW1, version = 6, pino = 71, ofs = 0x056803e4 in jffs2_add_fd_to_list
ino = 3, name = SW2, version = 3, pino = 71, ofs = 0x0e618740 <---mark obsolete by ino = 0, name = SW2, version = 7, pino = 71, ofs = 0x0568043c in jffs2_add_fd_to_list
ino = 4, name = SW3, version = 4, pino = 71, ofs = 0x0e618798 <---mark obsolete by ino = 0, name = SW3, version = 8, pino = 71, ofs = 0x05680494 in jffs2_add_fd_to_list
ino = 5, name = SW4, version = 5, pino = 71, ofs = 0x0e6187f0 <---mark obsolete by ino = 0, name = SW4, version = 9, pino = 71, ofs = 0x056804ec in jffs2_add_fd_to_list
---------------------------------------------------------------
There is a situation here, eraseblock(0x05680000 - 0x0569FFFF) could be all dirty, and erased before eraseblock(0x0e600000 - 0x0e61FFFF) which is not all dirty.
If this situation has happened, then /mnt/old/SW1 - /mnt/old/SW4 's deletion dirents has erased before /mnt/old/SW1 - /mnt/old/SW4 's dirents erase.
Now we remount jffs2, and will found these print.There /mnt/SW1(ino:2) and /mnt/old/SW1(ino:2) both exist, the two dirent both point to inode which ino number is 2.
However, /mnt/old(ino :71)'s deletion dirent is exist. Then in "jffs2_build_remove_unlinked_inode" will mark its child obsolete. Because /mnt/old/SW1(pino : 71, ino :2)
doesn't have its deletion dirent, then in "jffs2_build_remove_unlinked_inode" will execute to this line "child_ic->pino_nlink = 0;" and make inode(ino : 2) unlinked.
inode(ino:3,ino:4,ino:5) is same as inode(ino:2).
-----------------------------------------------------------------
ino = 2, name = SW1, version = 2, pino = 71, ofs = 0x0e6186e8
ino = 3, name = SW2, version = 3, pino = 71, ofs = 0x0e618740
ino = 4, name = SW3, version = 4, pino = 71, ofs = 0x0e618798
ino = 5, name = SW4, version = 5, pino = 71, ofs = 0x0e6187f0
child dir "SW1" (ino #2) of dir ino #71 appears to be a hard link
child dir "SW2" (ino #3) of dir ino #71 appears to be a hard link
child dir "SW3" (ino #4) of dir ino #71 appears to be a hard link
child dir "SW4" (ino #5) of dir ino #71 appears to be a hard link
------------------------------------------------------------------
Here, the state is below.
/mnt/SW1 <---dirent exist but inode's unlinked by "child_ic->pino_nlink = 0;"
/mnt/SW2 <---dirent exist but inode's unlinked by "child_ic->pino_nlink = 0;"
/mnt/SW3 <---dirent exist but inode's unlinked by "child_ic->pino_nlink = 0;"
/mnt/SW4 <---dirent exist but inode's unlinked by "child_ic->pino_nlink = 0;"
/mnt/old <--- deleted
/mnt/old/SW1 <--- doesn't have deletion dirent but mark obsolete in "jffs2_build_remove_unlinked_inode"
/mnt/old/SW2 <--- doesn't have deletion dirent but mark obsolete in "jffs2_build_remove_unlinked_inode"
/mnt/old/SW3 <--- doesn't have deletion dirent but mark obsolete in "jffs2_build_remove_unlinked_inode"
/mnt/old/SW4 <--- doesn't have deletion dirent but mark obsolete in "jffs2_build_remove_unlinked_inode"
So if we want to access /mnt/SW1, we will get this "no data nodes found for ino #2 ","./SW1: Input/output error", because there has no valid data nodes.
In this patch, we avoid to mark the deletion dirent of unlinked dirent's child obsolete in "jffs2_build_remove_unlinked_inode" and handle it in gc,
which could mark the deletion dirent obsolete safely.
Thanks
Best regards
--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2015-02-13 4:09 ` liu.song11
@ 2015-02-13 10:13 ` David Woodhouse
2015-02-13 11:28 ` David Woodhouse
0 siblings, 1 reply; 13+ messages in thread
From: David Woodhouse @ 2015-02-13 10:13 UTC (permalink / raw)
To: liu.song11; +Cc: jiang.xuexin, wang.bo116, linux-mtd, deng.chao1, cui.yunfeng
[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]
Thank you for the excellent analysis. However, I'm a little confused,
and I'm not sure we're focusing on the right thing.
Step five is to *unlink* the direction /mnt/old. Here's a
carefully-selected citation of your email...
On Fri, 2015-02-13 at 12:09 +0800, liu.song11@zte.com.cn wrote:
>
> 5. rmdir /mnt/old
> 6. mkdir /mnt/old
> 7. goto step 3
>
> In this loop, there only /mnt/old's ino number will changed.
>
> Before the problem happen, here we print the dirents' info(after step
> 3 and before step4).
> We can see，/mnt/old/SW1's pino is 71, that means /mnt/old's ino
> number is 71.
> The dirents all in same eraseblock(0x0e600000 - 0x0e61FFFF).
> -------------------------------------------------------------
...
> ino = 71, name = old, version = 299, pino = 1, ofs = 0x0e6186bc
> -------------------------------------------------------------
OK.
> After step5 and before step6, we print the dirents' info.
...
> ino = 0, name = old, version = 298, pino = 1, ofs = 0x0e6185c4
> ino = 71, name = old, version = 299, pino = 1, ofs = 0x0e6186bc
> ---------------------------------------------------------------
That looks wrong. You said you unlinked /mnt/old. But a deletion dirent
appeared *before* the creation, with an *older* version number?
Was that an older deletion dirent from a previous round, and you just
didn't include it in your previous printout (also cited above)? Is the
*new* deletion dirent with version = 300 somewhere else on the flash?
This is the problem, I think.
I don't *care* if the old dirents for <ino#71>/SW1 and <ino#71>/SW2 are
marked as obsolete or not. Because the directory <ino#71> *itself* as
unlinked and should be dead.
If <ino#71> is somehow ending up linked as /mnt/foo again when it should
have been unlinked, *that* is the problem we need to be looking at. The
zombie *children* of <ino#71> are just a symptom of its reincarnation.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2015-02-13 10:13 ` David Woodhouse
@ 2015-02-13 11:28 ` David Woodhouse
2015-03-12 3:05 ` liu.song11
0 siblings, 1 reply; 13+ messages in thread
From: David Woodhouse @ 2015-02-13 11:28 UTC (permalink / raw)
To: liu.song11; +Cc: jiang.xuexin, wang.bo116, linux-mtd, deng.chao1, cui.yunfeng
[-- Attachment #1: Type: text/plain, Size: 7054 bytes --]
On Fri, 2015-02-13 at 10:13 +0000, David Woodhouse wrote:
>
> If <ino#71> is somehow ending up linked as /mnt/foo again when it should
> have been unlinked, *that* is the problem we need to be looking at. The
> zombie *children* of <ino#71> are just a symptom of its reincarnation.
Ah, I think I understand a little better.
At the time the problem happens, it's too *early* in the scan/build
process. We don't yet *know* that ino#71 is dead, because we haven't yet
scanned the whole flash looking for dirents that might point to it.
In the problematic case, the directory SW1<#2> is first discovered when
we see its dirent from the *dead* directory <#71>, and we fill in
ic<2>->pino_nlink = 71.
And then *later* we find the *real* dirent which links SW1<#2> from the
root directory, and we hit that 'appears to be a hard link' message in
jffs2_build_inode_pass1().
The problem really comes when we *later* kill ino#71 because we realise
it doesn't have any valid dirents keeping it alive, and we *also* kill
all its children. Some of which weren't *really* supposed to be its
children :)
If we happened to hit the real dirent for SW1<#2> *first*, and the
dirent linking it from <#71> *later*, then we'd be OK.
This is all because of the NFS export support, which means we have to
know the parent inode# for directories. If we were still storing nlink
for directories as we do for plain files, we could fix this fairly
easily.
Your approach is to fix the garbage collection, to ensure that the old
deletion dirents "unlinking" SW1<#2> from the dead <ino#71> aren't
actually allowed to expire until all previous *positive* dirents linking
it into that dead parent directory have also expired — much like we
already do for garbage collection of deletion dirents in a live
directory.
That makes sense, but I'm not sure I like it very much. It's complex,
and it really does violate my sense of decency a little — if <ino#71> is
dead when it should *stay* dead, and not return to bother us.
I'll see if I can come up with a simpler solution purely within the scan
code. Can you test this, please?
diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index a3750f9..f5a9b0e 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -49,7 +49,8 @@ next_inode(int *i, struct jffs2_inode_cache *ic, struct jffs2_sb_info *c)
static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
- struct jffs2_inode_cache *ic)
+ struct jffs2_inode_cache *ic,
+ int *dir_hardlinks)
{
struct jffs2_full_dirent *fd;
@@ -71,16 +72,16 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
continue;
}
+ /* From this point, fd->raw is no longer used so we can set fd->ic */
+ fd->ic = child_ic;
+ child_ic->pino_nlink++;
+ /* If we appear (at this stage) to have hard-linked directories,
+ * set a flag to trigger a scan later */
if (fd->type == DT_DIR) {
- if (child_ic->pino_nlink) {
- JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u appears to be a hard link\n",
- fd->name, fd->ino, ic->ino);
- /* TODO: What do we do about it? */
- } else {
- child_ic->pino_nlink = ic->ino;
- }
- } else
- child_ic->pino_nlink++;
+ child_ic->flags |= INO_FLAGS_IS_DIR;
+ if (child_ic->pino_nlink > 1)
+ *dir_hardlinks = 1;
+ }
dbg_fsbuild("increased nlink for child \"%s\" (ino #%u)\n", fd->name, fd->ino);
/* Can't free scan_dents so far. We might need them in pass 2 */
@@ -94,8 +95,7 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
*/
static int jffs2_build_filesystem(struct jffs2_sb_info *c)
{
- int ret;
- int i;
+ int ret, i, dir_hardlinks = 0;
struct jffs2_inode_cache *ic;
struct jffs2_full_dirent *fd;
struct jffs2_full_dirent *dead_fds = NULL;
@@ -119,7 +119,7 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
/* Now scan the directory tree, increasing nlink according to every dirent found. */
for_each_inode(i, c, ic) {
if (ic->scan_dents) {
- jffs2_build_inode_pass1(c, ic);
+ jffs2_build_inode_pass1(c, ic, &dir_hardlinks);
cond_resched();
}
}
@@ -155,6 +155,20 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
}
dbg_fsbuild("pass 2a complete\n");
+
+ if (dir_hardlinks) {
+ /* If we detected directory hardlinks earlier, *hopefully*
+ * they are gone now because some of the links were from
+ * dead directories which still had some old dirents lying
+ * around and not yet garbage-collected, but which have
+ * been discarded above. So clear the pino_nlink field
+ * in each directory, so that the final scan below can
+ * print appropriate warnings. */
+ for_each_inode(i, c, ic) {
+ if (ic->flags & INO_FLAGS_IS_DIR)
+ ic->pino_nlink = 0;
+ }
+ }
dbg_fsbuild("freeing temporary data structures\n");
/* Finally, we can scan again and free the dirent structs */
@@ -162,6 +176,18 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
while(ic->scan_dents) {
fd = ic->scan_dents;
ic->scan_dents = fd->next;
+ /* We do use the pino_nlink field to count nlink of
+ * directories during fs build, so set it to the
+ * parent ino# now. Now that there's hopefully only
+ * one. */
+ if (fd->type == DT_DIR) {
+ if (dir_hardlinks && fd->ic->pino_nlink) {
+ JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
+ fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
+ /* Should we unlink it from its previous parent? */
+ }
+ fd->ic->pino_nlink = ic->ino;
+ }
jffs2_free_full_dirent(fd);
}
ic->scan_dents = NULL;
@@ -240,11 +266,7 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
/* Reduce nlink of the child. If it's now zero, stick it on the
dead_fds list to be cleaned up later. Else just free the fd */
-
- if (fd->type == DT_DIR)
- child_ic->pino_nlink = 0;
- else
- child_ic->pino_nlink--;
+ child_ic->pino_nlink--;
if (!child_ic->pino_nlink) {
dbg_fsbuild("inode #%u (\"%s\") now has no links; adding to dead_fds list.\n",
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index fa35ff7..0637271 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -194,6 +194,7 @@ struct jffs2_inode_cache {
#define INO_STATE_CLEARING 6 /* In clear_inode() */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
+#define INO_FLAGS_IS_DIR 0x02 /* is a directory */
#define RAWNODE_CLASS_INODE_CACHE 0
#define RAWNODE_CLASS_XATTR_DATUM 1
@@ -249,7 +250,10 @@ struct jffs2_readinode_info
struct jffs2_full_dirent
{
- struct jffs2_raw_node_ref *raw;
+ union {
+ struct jffs2_raw_node_ref *raw;
+ struct jffs2_inode_cache *ic; /* Just during part of build */
+ };
struct jffs2_full_dirent *next;
uint32_t version;
uint32_t ino; /* == zero for unlink */
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2015-02-13 11:28 ` David Woodhouse
@ 2015-03-12 3:05 ` liu.song11
0 siblings, 0 replies; 13+ messages in thread
From: liu.song11 @ 2015-03-12 3:05 UTC (permalink / raw)
To: David Woodhouse
Cc: jiang.xuexin, wang.bo116, linux-mtd, deng.chao1, cui.yunfeng
Hi David，
Apologies for the delay of this reply that caused by taken holiday of the Spring Festival.
I have tested your codes and found it can work very well. I think this solution is very
subtle, simple and reasonable, I really like this solution, thanks a lot.
Best Regards
David Woodhouse <dwmw2@infradead.org> wrote on 2015-02-13 19:28:01:
> From: David Woodhouse <dwmw2@infradead.org>
> To: liu.song11@zte.com.cn,
> Cc: cui.yunfeng@zte.com.cn, deng.chao1@zte.com.cn, jiang.xuexin@zte.com.cn, linux-mtd@lists.infradead.org, wang.bo116@zte.com.cn
> Date: 2015-02-13 19:28
> Subject: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
>
> On Fri, 2015-02-13 at 10:13 +0000, David Woodhouse wrote:
> >
> > If <ino#71> is somehow ending up linked as /mnt/foo again when it should
> > have been unlinked, *that* is the problem we need to be looking at. The
> > zombie *children* of <ino#71> are just a symptom of its reincarnation.
>
> Ah, I think I understand a little better.
>
> At the time the problem happens, it's too *early* in the scan/build
> process. We don't yet *know* that ino#71 is dead, because we haven't yet
> scanned the whole flash looking for dirents that might point to it.
>
> In the problematic case, the directory SW1<#2> is first discovered when
> we see its dirent from the *dead* directory <#71>, and we fill in
> ic<2>->pino_nlink = 71.
>
> And then *later* we find the *real* dirent which links SW1<#2> from the
> root directory, and we hit that 'appears to be a hard link' message in
> jffs2_build_inode_pass1().
>
> The problem really comes when we *later* kill ino#71 because we realise
> it doesn't have any valid dirents keeping it alive, and we *also* kill
> all its children. Some of which weren't *really* supposed to be its
> children :)
>
> If we happened to hit the real dirent for SW1<#2> *first*, and the
> dirent linking it from <#71> *later*, then we'd be OK.
>
> This is all because of the NFS export support, which means we have to
> know the parent inode# for directories. If we were still storing nlink
> for directories as we do for plain files, we could fix this fairly
> easily.
>
> Your approach is to fix the garbage collection, to ensure that the old
> deletion dirents "unlinking" SW1<#2> from the dead <ino#71> aren't
> actually allowed to expire until all previous *positive* dirents linking
> it into that dead parent directory have also expired ― much like we
> already do for garbage collection of deletion dirents in a live
> directory.
>
> That makes sense, but I'm not sure I like it very much. It's complex,
> and it really does violate my sense of decency a little ― if <ino#71> is
> dead when it should *stay* dead, and not return to bother us.
>
> I'll see if I can come up with a simpler solution purely within the scan
> code. Can you test this, please?
>
> diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
> index a3750f9..f5a9b0e 100644
> --- a/fs/jffs2/build.c
> +++ b/fs/jffs2/build.c
> @@ -49,7 +49,8 @@ next_inode(int *i, struct jffs2_inode_cache *ic, struct jffs2_sb_info *c)
>
>
> static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
> - struct jffs2_inode_cache *ic)
> + struct jffs2_inode_cache *ic,
> + int *dir_hardlinks)
> {
> struct jffs2_full_dirent *fd;
>
> @@ -71,16 +72,16 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
> continue;
> }
>
> + /* From this point, fd->raw is no longer used so we can set fd->ic */
> + fd->ic = child_ic;
> + child_ic->pino_nlink++;
> + /* If we appear (at this stage) to have hard-linked directories,
> + * set a flag to trigger a scan later */
> if (fd->type == DT_DIR) {
> - if (child_ic->pino_nlink) {
> - JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u appears to be a hard link\n",
> - fd->name, fd->ino, ic->ino);
> - /* TODO: What do we do about it? */
> - } else {
> - child_ic->pino_nlink = ic->ino;
> - }
> - } else
> - child_ic->pino_nlink++;
> + child_ic->flags |= INO_FLAGS_IS_DIR;
> + if (child_ic->pino_nlink > 1)
> + *dir_hardlinks = 1;
> + }
>
> dbg_fsbuild("increased nlink for child \"%s\" (ino #%u)\n", fd->name, fd->ino);
> /* Can't free scan_dents so far. We might need them in pass 2 */
> @@ -94,8 +95,7 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
> */
> static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> {
> - int ret;
> - int i;
> + int ret, i, dir_hardlinks = 0;
> struct jffs2_inode_cache *ic;
> struct jffs2_full_dirent *fd;
> struct jffs2_full_dirent *dead_fds = NULL;
> @@ -119,7 +119,7 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> /* Now scan the directory tree, increasing nlink according to every dirent found. */
> for_each_inode(i, c, ic) {
> if (ic->scan_dents) {
> - jffs2_build_inode_pass1(c, ic);
> + jffs2_build_inode_pass1(c, ic, &dir_hardlinks);
> cond_resched();
> }
> }
> @@ -155,6 +155,20 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> }
>
> dbg_fsbuild("pass 2a complete\n");
> +
> + if (dir_hardlinks) {
> + /* If we detected directory hardlinks earlier, *hopefully*
> + * they are gone now because some of the links were from
> + * dead directories which still had some old dirents lying
> + * around and not yet garbage-collected, but which have
> + * been discarded above. So clear the pino_nlink field
> + * in each directory, so that the final scan below can
> + * print appropriate warnings. */
> + for_each_inode(i, c, ic) {
> + if (ic->flags & INO_FLAGS_IS_DIR)
> + ic->pino_nlink = 0;
> + }
> + }
> dbg_fsbuild("freeing temporary data structures\n");
>
> /* Finally, we can scan again and free the dirent structs */
> @@ -162,6 +176,18 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> while(ic->scan_dents) {
> fd = ic->scan_dents;
> ic->scan_dents = fd->next;
> + /* We do use the pino_nlink field to count nlink of
> + * directories during fs build, so set it to the
> + * parent ino# now. Now that there's hopefully only
> + * one. */
> + if (fd->type == DT_DIR) {
> + if (dir_hardlinks && fd->ic->pino_nlink) {
> + JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
> + fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
> + /* Should we unlink it from its previous parent? */
> + }
> + fd->ic->pino_nlink = ic->ino;
> + }
> jffs2_free_full_dirent(fd);
> }
> ic->scan_dents = NULL;
> @@ -240,11 +266,7 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
>
> /* Reduce nlink of the child. If it's now zero, stick it on the
> dead_fds list to be cleaned up later. Else just free the fd */
> -
> - if (fd->type == DT_DIR)
> - child_ic->pino_nlink = 0;
> - else
> - child_ic->pino_nlink--;
> + child_ic->pino_nlink--;
>
> if (!child_ic->pino_nlink) {
> dbg_fsbuild("inode #%u (\"%s\") now has no links; adding to dead_fds list.\n",
> diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
> index fa35ff7..0637271 100644
> --- a/fs/jffs2/nodelist.h
> +++ b/fs/jffs2/nodelist.h
> @@ -194,6 +194,7 @@ struct jffs2_inode_cache {
> #define INO_STATE_CLEARING 6 /* In clear_inode() */
>
> #define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
> +#define INO_FLAGS_IS_DIR 0x02 /* is a directory */
>
> #define RAWNODE_CLASS_INODE_CACHE 0
> #define RAWNODE_CLASS_XATTR_DATUM 1
> @@ -249,7 +250,10 @@ struct jffs2_readinode_info
>
> struct jffs2_full_dirent
> {
> - struct jffs2_raw_node_ref *raw;
> + union {
> + struct jffs2_raw_node_ref *raw;
> + struct jffs2_inode_cache *ic; /* Just during part of build */
> + };
> struct jffs2_full_dirent *next;
> uint32_t version;
> uint32_t ino; /* == zero for unlink */
>
>
> --
> dwmw2
--------------------------------------------------------
ZTE Information Security Notice: The information contained in this mail (and any attachment transmitted herewith) is privileged and confidential and is intended for the exclusive use of the addressee(s). If you are not an intended recipient, any disclosure, reproduction, distribution or other dissemination or use of the information contained is strictly prohibited. If you have received this mail in error, please delete it and notify us immediately.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
[not found] <OF2CCB8417.29770A56-ON48257EE5.0004DBEE-48257EE5.0004E5CC@LocalDomain>
@ 2015-10-26 3:19 ` deng.chao1
2016-02-01 14:21 ` David Woodhouse
0 siblings, 1 reply; 13+ messages in thread
From: deng.chao1 @ 2015-10-26 3:19 UTC (permalink / raw)
To: David Woodhouse; +Cc: linux-mtd, liu.song11, cui.yunfeng, jiang.xuexin
I have recently found a bug in this patch.
When we mount jffs2 file system, call function jffs2_build_filesystem->jffs2_build_inode_pass1.
I list code of jffs2_build_inode_pass1 which has already apply the patch as following :
static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
struct jffs2_inode_cache *ic,
int *dir_hardlinks)
{
struct jffs2_full_dirent *fd;
dbg_fsbuild("building directory inode #%u\n", ic->ino);
/* For each child, increase nlink */
for(fd = ic->scan_dents; fd; fd = fd->next) {
struct jffs2_inode_cache *child_ic;
if (!fd->ino)
continue;
/* we can get high latency here with huge directories */
child_ic = jffs2_get_ino_cache(c, fd->ino);
if (!child_ic) {
dbg_fsbuild("child \"%s\" (ino #%u) of dir ino #%u doesn't exist!\n",
fd->name, fd->ino, ic->ino);
jffs2_mark_node_obsolete(c, fd->raw);
continue;
}
/* From this point, fd->raw is no longer used so we can set fd->ic */
fd->ic = child_ic;
child_ic->pino_nlink++;
/* If we appear (at this stage) to have hard-linked directories,
* set a flag to trigger a scan later */
if (fd->type == DT_DIR) {
child_ic->flags |= INO_FLAGS_IS_DIR;
if (child_ic->pino_nlink > 1)
*dir_hardlinks = 1;
}
dbg_fsbuild("increased nlink for child \"%s\" (ino #%u)\n", fd->name, fd->ino);
/* Can't free scan_dents so far. We might need them in pass 2 */
}
}
In this function, when jffs2_get_ino_cache can't find child_ic for child fd and return NULL, the processing loop will continue and leave this child's fd->ic remains as fd->raw because they are union in struct jffs2_full_dirent:
struct jffs2_full_dirent
{
- struct jffs2_raw_node_ref *raw;
+ union {
+ struct jffs2_raw_node_ref *raw;
+ struct jffs2_inode_cache *ic; /* Just during part of build */
+ };
struct jffs2_full_dirent *next;
uint32_t version;
uint32_t ino; /* == zero for unlink */
Then following code in jffs2_build_filesystem:
static int jffs2_build_filesystem(struct jffs2_sb_info *c)
{
...
/* Finally, we can scan again and free the dirent structs */
for_each_inode(i, c, ic) {
while(ic->scan_dents) {
fd = ic->scan_dents;
ic->scan_dents = fd->next;
/* We do use the pino_nlink field to count nlink of
* directories during fs build, so set it to the
* parent ino# now. Now that there's hopefully only
* one. */
if (fd->type == DT_DIR) {
if (dir_hardlinks && fd->ic->pino_nlink) {
JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
/* Should we unlink it from its previous parent? */
}
fd->ic->pino_nlink = ic->ino;
}
jffs2_free_full_dirent(fd);
}
ic->scan_dents = NULL;
cond_resched();
}
...
}
The code here "fd->ic->pino_nlink = ic->ino;" will access wrong pointer when fd->ic remains as fd->raw.
To solve this problem, we can let fd->ic and fd->raw do not share the union, and make a mark by setting fd->ic=0 when jffs2_get_ino_cache can't find child_ic for child fd in function jffs2_build_inode_pass1, so that we can only do "fd->ic->pino_nlink = ic->ino" when fd->ic!=0.
And I have a question, what situation will cause jffs2_get_ino_cache can't find child_ic for child fd in jffs2_build_inode_pass1? This really happens.
I have thought it over, and not figure it out ;(
LiuSong10144506/user/zte_ltd
2015-10-21 08:53 To
DengChao153859/user/zte_ltd@zte_ltd,
cc
Subject
Fw: [PATCH]jffs2:bugfix for should not appeared directory hard link
----- Forwarded by LiuSong10144506/user/zte_ltd on 2015-10-21 08:53 -----
From: David Woodhouse <dwmw2@infradead.org>
To: liu.song11@zte.com.cn,
Cc: cui.yunfeng@zte.com.cn, deng.chao1@zte.com.cn, jiang.xuexin@zte.com.cn, linux-mtd@lists.infradead.org, wang.bo116@zte.com.cn
Date: 2015-02-13 19:28
Subject: Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
On Fri, 2015-02-13 at 10:13 +0000, David Woodhouse wrote:
>
> If <ino#71> is somehow ending up linked as /mnt/foo again when it should
> have been unlinked, *that* is the problem we need to be looking at. The
> zombie *children* of <ino#71> are just a symptom of its reincarnation.
Ah, I think I understand a little better.
At the time the problem happens, it's too *early* in the scan/build
process. We don't yet *know* that ino#71 is dead, because we haven't yet
scanned the whole flash looking for dirents that might point to it.
In the problematic case, the directory SW1<#2> is first discovered when
we see its dirent from the *dead* directory <#71>, and we fill in
ic<2>->pino_nlink = 71.
And then *later* we find the *real* dirent which links SW1<#2> from the
root directory, and we hit that 'appears to be a hard link' message in
jffs2_build_inode_pass1().
The problem really comes when we *later* kill ino#71 because we realise
it doesn't have any valid dirents keeping it alive, and we *also* kill
all its children. Some of which weren't *really* supposed to be its
children :)
If we happened to hit the real dirent for SW1<#2> *first*, and the
dirent linking it from <#71> *later*, then we'd be OK.
This is all because of the NFS export support, which means we have to
know the parent inode# for directories. If we were still storing nlink
for directories as we do for plain files, we could fix this fairly
easily.
Your approach is to fix the garbage collection, to ensure that the old
deletion dirents "unlinking" SW1<#2> from the dead <ino#71> aren't
actually allowed to expire until all previous *positive* dirents linking
it into that dead parent directory have also expired ― much like we
already do for garbage collection of deletion dirents in a live
directory.
That makes sense, but I'm not sure I like it very much. It's complex,
and it really does violate my sense of decency a little ― if <ino#71> is
dead when it should *stay* dead, and not return to bother us.
I'll see if I can come up with a simpler solution purely within the scan
code. Can you test this, please?
diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index a3750f9..f5a9b0e 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -49,7 +49,8 @@ next_inode(int *i, struct jffs2_inode_cache *ic, struct jffs2_sb_info *c)
static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
- struct jffs2_inode_cache *ic)
+ struct jffs2_inode_cache *ic,
+ int *dir_hardlinks)
{
struct jffs2_full_dirent *fd;
@@ -71,16 +72,16 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
continue;
}
+ /* From this point, fd->raw is no longer used so we can set fd->ic */
+ fd->ic = child_ic;
+ child_ic->pino_nlink++;
+ /* If we appear (at this stage) to have hard-linked directories,
+ * set a flag to trigger a scan later */
if (fd->type == DT_DIR) {
- if (child_ic->pino_nlink) {
- JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u appears to be a hard link\n",
- fd->name, fd->ino, ic->ino);
- /* TODO: What do we do about it? */
- } else {
- child_ic->pino_nlink = ic->ino;
- }
- } else
- child_ic->pino_nlink++;
+ child_ic->flags |= INO_FLAGS_IS_DIR;
+ if (child_ic->pino_nlink > 1)
+ *dir_hardlinks = 1;
+ }
dbg_fsbuild("increased nlink for child \"%s\" (ino #%u)\n", fd->name, fd->ino);
/* Can't free scan_dents so far. We might need them in pass 2 */
@@ -94,8 +95,7 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
*/
static int jffs2_build_filesystem(struct jffs2_sb_info *c)
{
- int ret;
- int i;
+ int ret, i, dir_hardlinks = 0;
struct jffs2_inode_cache *ic;
struct jffs2_full_dirent *fd;
struct jffs2_full_dirent *dead_fds = NULL;
@@ -119,7 +119,7 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
/* Now scan the directory tree, increasing nlink according to every dirent found. */
for_each_inode(i, c, ic) {
if (ic->scan_dents) {
- jffs2_build_inode_pass1(c, ic);
+ jffs2_build_inode_pass1(c, ic, &dir_hardlinks);
cond_resched();
}
}
@@ -155,6 +155,20 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
}
dbg_fsbuild("pass 2a complete\n");
+
+ if (dir_hardlinks) {
+ /* If we detected directory hardlinks earlier, *hopefully*
+ * they are gone now because some of the links were from
+ * dead directories which still had some old dirents lying
+ * around and not yet garbage-collected, but which have
+ * been discarded above. So clear the pino_nlink field
+ * in each directory, so that the final scan below can
+ * print appropriate warnings. */
+ for_each_inode(i, c, ic) {
+ if (ic->flags & INO_FLAGS_IS_DIR)
+ ic->pino_nlink = 0;
+ }
+ }
dbg_fsbuild("freeing temporary data structures\n");
/* Finally, we can scan again and free the dirent structs */
@@ -162,6 +176,18 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
while(ic->scan_dents) {
fd = ic->scan_dents;
ic->scan_dents = fd->next;
+ /* We do use the pino_nlink field to count nlink of
+ * directories during fs build, so set it to the
+ * parent ino# now. Now that there's hopefully only
+ * one. */
+ if (fd->type == DT_DIR) {
+ if (dir_hardlinks && fd->ic->pino_nlink) {
+ JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
+ fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
+ /* Should we unlink it from its previous parent? */
+ }
+ fd->ic->pino_nlink = ic->ino;
+ }
jffs2_free_full_dirent(fd);
}
ic->scan_dents = NULL;
@@ -240,11 +266,7 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
/* Reduce nlink of the child. If it's now zero, stick it on the
dead_fds list to be cleaned up later. Else just free the fd */
-
- if (fd->type == DT_DIR)
- child_ic->pino_nlink = 0;
- else
- child_ic->pino_nlink--;
+ child_ic->pino_nlink--;
if (!child_ic->pino_nlink) {
dbg_fsbuild("inode #%u (\"%s\") now has no links; adding to dead_fds list.\n",
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index fa35ff7..0637271 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -194,6 +194,7 @@ struct jffs2_inode_cache {
#define INO_STATE_CLEARING 6 /* In clear_inode() */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
+#define INO_FLAGS_IS_DIR 0x02 /* is a directory */
#define RAWNODE_CLASS_INODE_CACHE 0
#define RAWNODE_CLASS_XATTR_DATUM 1
@@ -249,7 +250,10 @@ struct jffs2_readinode_info
struct jffs2_full_dirent
{
- struct jffs2_raw_node_ref *raw;
+ union {
+ struct jffs2_raw_node_ref *raw;
+ struct jffs2_inode_cache *ic; /* Just during part of build */
+ };
struct jffs2_full_dirent *next;
uint32_t version;
uint32_t ino; /* == zero for unlink */
--
dwmw2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH]jffs2:bugfix for should not appeared directory hard link
2015-10-26 3:19 ` deng.chao1
@ 2016-02-01 14:21 ` David Woodhouse
0 siblings, 0 replies; 13+ messages in thread
From: David Woodhouse @ 2016-02-01 14:21 UTC (permalink / raw)
To: deng.chao1; +Cc: jiang.xuexin, linux-mtd, liu.song11, cui.yunfeng
[-- Attachment #1: Type: text/plain, Size: 10212 bytes --]
On Mon, 2015-10-26 at 11:19 +0800, deng.chao1@zte.com.cn wrote:
> I have recently found a bug in this patch.
> When we mount jffs2 file system, call function
> jffs2_build_filesystem->jffs2_build_inode_pass1.
...
> In this function, when jffs2_get_ino_cache can't find child_ic for
> child fd and return NULL, the processing loop will continue and leave
> this child's fd->ic remains as fd->raw because they are union in
> struct jffs2_full_dirent:
> Then following code in jffs2_build_filesystem:
> static int jffs2_build_filesystem(struct jffs2_sb_info *c)
> {
> ...
> /* Finally, we can scan again and free the dirent structs */
> for_each_inode(i, c, ic) {
> while(ic->scan_dents) {
> fd = ic->scan_dents;
> ic->scan_dents = fd->next;
> /* We do use the pino_nlink field to count nlink of
> * directories during fs build, so set it to the
> * parent ino# now. Now that there's hopefully only
> * one. */
> if (fd->type == DT_DIR) {
> if (dir_hardlinks && fd->ic->pino_nlink) {
> JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
> fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
> /* Should we unlink it from its previous parent? */
> }
> fd->ic->pino_nlink = ic->ino;
> }
> jffs2_free_full_dirent(fd);
> }
> ic->scan_dents = NULL;
> cond_resched();
> }
> ...
> }
> The code here "fd->ic->pino_nlink = ic->ino;" will access wrong
> pointer when fd->ic remains as fd->raw.
> To solve this problem, we can let fd->ic and fd->raw do not share the
> union, and make a mark by setting fd->ic=0 when jffs2_get_ino_cache
> can't find child_ic for child fd in function jffs2_build_inode_pass1,
> so that we can only do "fd->ic->pino_nlink = ic->ino" when fd->ic!=0.
Alternatively, we *do* continue to save memory by having that union,
and we just ensure that we *do* clear fd->ic in the case where
jffs2_get_ino_cache() fails. Then cope with it being NULL. New patch
below.
> And I have a question, what situation will cause jffs2_get_ino_cache
> can't find child_ic for child fd in jffs2_build_inode_pass1? This
> really happens.
> I have thought it over, and not figure it out ;(
It can happen if the parent directory has an entry indicating a child
(either file or directory) which does't actually exist — there are no
physical nodes on the medium which belong to that child inode.
I suspect that this should only ever happen when the parent directory
itself has been deleted but not yet completely obsoleted from the
flash. Which is precisely the situation you originally described, isn't
it?
----
From: David Woodhouse <David.Woodhouse@intel.com>
Subject: [PATCH] Fix directory hardlinks from deleted directories
When a directory is deleted, we don't take too much care about killing off
all the dirents that belong to it — on the basis that on remount, the scan
will conclude that the directory is dead anyway.
This doesn't work though, when the deleted directory contained a child
directory which was moved *out*. In the early stages of the fs build
we can then end up with an apparent hard link, with the child directory
appearing both in its true location, and as a child of the original
directory which are this stage of the mount process we don't *yet* know
is defunct.
To resolve this, take out the early special-casing of the "directories
shall not have hard links" rule in jffs2_build_inode_pass1(), and let the
normal nlink processing happen for directories as well as other inodes.
Then later in the build process we can set ic->pino_nlink to the parent
inode#, as is required for directories during normal operaton, instead
of the nlink. And complain only *then* about hard links which are still
in evidence even after killing off all the unreachable paths.
Reported-by: Liu Song <liu.song11@zte.com.cn>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
fs/jffs2/build.c | 75 ++++++++++++++++++++++++++++++++++++++++-------------
fs/jffs2/nodelist.h | 6 ++++-
2 files changed, 62 insertions(+), 19 deletions(-)
diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index a3750f9..c1f0494 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -49,7 +49,8 @@ next_inode(int *i, struct jffs2_inode_cache *ic, struct jffs2_sb_info *c)
static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
- struct jffs2_inode_cache *ic)
+ struct jffs2_inode_cache *ic,
+ int *dir_hardlinks)
{
struct jffs2_full_dirent *fd;
@@ -68,19 +69,21 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
dbg_fsbuild("child \"%s\" (ino #%u) of dir ino #%u doesn't exist!\n",
fd->name, fd->ino, ic->ino);
jffs2_mark_node_obsolete(c, fd->raw);
+ /* Clear the ic/raw union so it doesn't cause problems later. */
+ fd->ic = NULL;
continue;
}
+ /* From this point, fd->raw is no longer used so we can set fd->ic */
+ fd->ic = child_ic;
+ child_ic->pino_nlink++;
+ /* If we appear (at this stage) to have hard-linked directories,
+ * set a flag to trigger a scan later */
if (fd->type == DT_DIR) {
- if (child_ic->pino_nlink) {
- JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u appears to be a hard link\n",
- fd->name, fd->ino, ic->ino);
- /* TODO: What do we do about it? */
- } else {
- child_ic->pino_nlink = ic->ino;
- }
- } else
- child_ic->pino_nlink++;
+ child_ic->flags |= INO_FLAGS_IS_DIR;
+ if (child_ic->pino_nlink > 1)
+ *dir_hardlinks = 1;
+ }
dbg_fsbuild("increased nlink for child \"%s\" (ino #%u)\n", fd->name, fd->ino);
/* Can't free scan_dents so far. We might need them in pass 2 */
@@ -94,8 +97,7 @@ static void jffs2_build_inode_pass1(struct jffs2_sb_info *c,
*/
static int jffs2_build_filesystem(struct jffs2_sb_info *c)
{
- int ret;
- int i;
+ int ret, i, dir_hardlinks = 0;
struct jffs2_inode_cache *ic;
struct jffs2_full_dirent *fd;
struct jffs2_full_dirent *dead_fds = NULL;
@@ -119,7 +121,7 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
/* Now scan the directory tree, increasing nlink according to every dirent found. */
for_each_inode(i, c, ic) {
if (ic->scan_dents) {
- jffs2_build_inode_pass1(c, ic);
+ jffs2_build_inode_pass1(c, ic, &dir_hardlinks);
cond_resched();
}
}
@@ -155,6 +157,20 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
}
dbg_fsbuild("pass 2a complete\n");
+
+ if (dir_hardlinks) {
+ /* If we detected directory hardlinks earlier, *hopefully*
+ * they are gone now because some of the links were from
+ * dead directories which still had some old dirents lying
+ * around and not yet garbage-collected, but which have
+ * been discarded above. So clear the pino_nlink field
+ * in each directory, so that the final scan below can
+ * print appropriate warnings. */
+ for_each_inode(i, c, ic) {
+ if (ic->flags & INO_FLAGS_IS_DIR)
+ ic->pino_nlink = 0;
+ }
+ }
dbg_fsbuild("freeing temporary data structures\n");
/* Finally, we can scan again and free the dirent structs */
@@ -162,6 +178,33 @@ static int jffs2_build_filesystem(struct jffs2_sb_info *c)
while(ic->scan_dents) {
fd = ic->scan_dents;
ic->scan_dents = fd->next;
+ /* We do use the pino_nlink field to count nlink of
+ * directories during fs build, so set it to the
+ * parent ino# now. Now that there's hopefully only
+ * one. */
+ if (fd->type == DT_DIR) {
+ if (!fd->ic) {
+ /* We'll have complained about it and marked the coresponding
+ raw node obsolete already. Just skip it. */
+ continue;
+ }
+
+ /* We *have* to have set this in jffs2_build_inode_pass1() */
+ BUG_ON(!(fd->ic->flags & INO_FLAGS_IS_DIR));
+
+ /* We clear ic->pino_nlink ∀ directories' ic *only* if dir_hardlinks
+ * is set. Otherwise, we know this should never trigger anyway, so
+ * we don't do the check. And ic->pino_nlink still contains the nlink
+ * value (which is 1). */
+ if (dir_hardlinks && fd->ic->pino_nlink) {
+ JFFS2_ERROR("child dir \"%s\" (ino #%u) of dir ino #%u is also hard linked from dir ino #%u\n",
+ fd->name, fd->ino, ic->ino, fd->ic->pino_nlink);
+ /* Should we unlink it from its previous parent? */
+ }
+
+ /* For directories, ic->pino_nlink holds that parent inode # */
+ fd->ic->pino_nlink = ic->ino;
+ }
jffs2_free_full_dirent(fd);
}
ic->scan_dents = NULL;
@@ -240,11 +283,7 @@ static void jffs2_build_remove_unlinked_inode(struct jffs2_sb_info *c,
/* Reduce nlink of the child. If it's now zero, stick it on the
dead_fds list to be cleaned up later. Else just free the fd */
-
- if (fd->type == DT_DIR)
- child_ic->pino_nlink = 0;
- else
- child_ic->pino_nlink--;
+ child_ic->pino_nlink--;
if (!child_ic->pino_nlink) {
dbg_fsbuild("inode #%u (\"%s\") now has no links; adding to dead_fds list.\n",
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index fa35ff7..0637271 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -194,6 +194,7 @@ struct jffs2_inode_cache {
#define INO_STATE_CLEARING 6 /* In clear_inode() */
#define INO_FLAGS_XATTR_CHECKED 0x01 /* has no duplicate xattr_ref */
+#define INO_FLAGS_IS_DIR 0x02 /* is a directory */
#define RAWNODE_CLASS_INODE_CACHE 0
#define RAWNODE_CLASS_XATTR_DATUM 1
@@ -249,7 +250,10 @@ struct jffs2_readinode_info
struct jffs2_full_dirent
{
- struct jffs2_raw_node_ref *raw;
+ union {
+ struct jffs2_raw_node_ref *raw;
+ struct jffs2_inode_cache *ic; /* Just during part of build */
+ };
struct jffs2_full_dirent *next;
uint32_t version;
uint32_t ino; /* == zero for unlink */
--
2.5.0
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-02-01 14:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-24 9:26 [PATCH]jffs2:bugfix for should not appeared directory hard link liu.song11
2014-11-05 11:45 ` Brian Norris
2014-11-11 7:12 ` liu.song11
2014-11-12 6:45 ` Liu Song
2014-12-17 1:53 ` Brian Norris
2014-12-17 2:02 ` nick
2015-02-12 13:43 ` David Woodhouse
2015-02-13 4:09 ` liu.song11
2015-02-13 10:13 ` David Woodhouse
2015-02-13 11:28 ` David Woodhouse
2015-03-12 3:05 ` liu.song11
[not found] <OF2CCB8417.29770A56-ON48257EE5.0004DBEE-48257EE5.0004E5CC@LocalDomain>
2015-10-26 3:19 ` deng.chao1
2016-02-01 14:21 ` David Woodhouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox