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 at 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 at infradead.org>
To: liu.song11 at zte.com.cn,
Cc: cui.yunfeng at zte.com.cn, deng.chao1 at zte.com.cn, jiang.xuexin at zte.com.cn, linux-mtd at lists.infradead.org, wang.bo116 at 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