* e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() @ 2011-11-03 6:45 Niu 2011-11-03 14:47 ` Eric Sandeen 0 siblings, 1 reply; 6+ messages in thread From: Niu @ 2011-11-03 6:45 UTC (permalink / raw) To: tytso; +Cc: linux-ext4 hello, Ted In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs 'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration, but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop. I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you. >From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001 From: Niu Yawei <niu@whamcloud.com> Date: Wed, 2 Nov 2011 04:31:11 +0800 Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device() The ext2fs_find_block_device() should stop searching when the directory depth reaching a certain threshold, otherwise, it could run into infinite loop if there are loop links in the device directory. Signed-off-by: Niu Yawei <niu@whamcloud.com> --- lib/ext2fs/finddev.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c index 13ef14b..61f9754 100644 --- a/lib/ext2fs/finddev.c +++ b/lib/ext2fs/finddev.c @@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device) struct dir_list *list = 0, *new_list = 0; struct dir_list *current; char *ret_path = 0; + int level = 0; /* * Add the starting directories to search... @@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device) if (list == 0) { list = new_list; new_list = 0; + /* Avoid infinite loop */ + if (++level > 5) + break; } } free_dirlist(&list); -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() 2011-11-03 6:45 e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() Niu @ 2011-11-03 14:47 ` Eric Sandeen 2011-11-03 15:27 ` Theodore Tso 0 siblings, 1 reply; 6+ messages in thread From: Eric Sandeen @ 2011-11-03 14:47 UTC (permalink / raw) To: Niu; +Cc: tytso, linux-ext4 On 11/3/11 1:45 AM, Niu wrote: > hello, Ted > > In my rhel5 system, there are lots of loop links in the /dev/.udev/failed folder, which makes the e2fsprogs > 'make check' stuck in 't_ext_jnl_rm'. I'm not sure if the loop links are generated by udev defect or misconfiguration, > but anyway, I think the ext2fs_find_block_device() should do some sanity check to avoid infinite loop. > > I made a simple patch which breaks the loop when depth reaching 5 in ext2fs_find_block_device(), any comments? Thank you. My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds? I guess this function isn't very frequently called outside of make check though, just from tune2fs' remove_journal_device() (for external journals I guess?) so I suppose it's not too worrisome. -Eric > From 5ef3b82266e7fb5edbce4febc4924b0beaccacf1 Mon Sep 17 00:00:00 2001 > From: Niu Yawei <niu@whamcloud.com> > Date: Wed, 2 Nov 2011 04:31:11 +0800 > Subject: [PATCH] Avoid infinite loop in ext2fs_find_block_device() > > The ext2fs_find_block_device() should stop searching when the > directory depth reaching a certain threshold, otherwise, it could > run into infinite loop if there are loop links in the device > directory. > > Signed-off-by: Niu Yawei <niu@whamcloud.com> > --- > lib/ext2fs/finddev.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c > index 13ef14b..61f9754 100644 > --- a/lib/ext2fs/finddev.c > +++ b/lib/ext2fs/finddev.c > @@ -128,6 +128,7 @@ char *ext2fs_find_block_device(dev_t device) > struct dir_list *list = 0, *new_list = 0; > struct dir_list *current; > char *ret_path = 0; > + int level = 0; > > /* > * Add the starting directories to search... > @@ -154,6 +155,9 @@ char *ext2fs_find_block_device(dev_t device) > if (list == 0) { > list = new_list; > new_list = 0; > + /* Avoid infinite loop */ > + if (++level > 5) > + break; > } > } > free_dirlist(&list); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() 2011-11-03 14:47 ` Eric Sandeen @ 2011-11-03 15:27 ` Theodore Tso 2011-11-04 12:50 ` Niu 0 siblings, 1 reply; 6+ messages in thread From: Theodore Tso @ 2011-11-03 15:27 UTC (permalink / raw) To: Eric Sandeen; +Cc: Theodore Tso, Niu, linux-ext4 On Nov 3, 2011, at 10:47 AM, Eric Sandeen wrote: > > My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? > Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds? Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c. -- Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() 2011-11-03 15:27 ` Theodore Tso @ 2011-11-04 12:50 ` Niu 2011-11-20 4:18 ` Ted Ts'o 2011-11-20 8:17 ` Christian Kujau 0 siblings, 2 replies; 6+ messages in thread From: Niu @ 2011-11-04 12:50 UTC (permalink / raw) To: Theodore Tso; +Cc: Eric Sandeen, linux-ext4 >From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001 From: Niu Yawei <niu@whamcloud.com> Date: Wed, 2 Nov 2011 04:31:11 +0800 Subject: [PATCH] e2fsprogs: maximum nested link count Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count not exceeding it in ext2fs_find_block_device() and follow_link(). Signed-off-by: Niu Yawei <niu@whamcloud.com> --- lib/ext2fs/ext2fsP.h | 2 ++ lib/ext2fs/finddev.c | 5 +++++ lib/ext2fs/namei.c | 3 ++- 3 files changed, 9 insertions(+), 1 deletions(-) diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h index b182d7f..82e1ba0 100644 --- a/lib/ext2fs/ext2fsP.h +++ b/lib/ext2fs/ext2fsP.h @@ -11,6 +11,8 @@ #include "ext2fs.h" +#define EXT2FS_MAX_NESTED_LINKS 8 + /* * Badblocks list */ diff --git a/lib/ext2fs/finddev.c b/lib/ext2fs/finddev.c index 13ef14b..311608d 100644 --- a/lib/ext2fs/finddev.c +++ b/lib/ext2fs/finddev.c @@ -34,6 +34,7 @@ #include "ext2_fs.h" #include "ext2fs.h" +#include "ext2fsP.h" struct dir_list { char *name; @@ -128,6 +129,7 @@ char *ext2fs_find_block_device(dev_t device) struct dir_list *list = 0, *new_list = 0; struct dir_list *current; char *ret_path = 0; + int level = 0; /* * Add the starting directories to search... @@ -154,6 +156,9 @@ char *ext2fs_find_block_device(dev_t device) if (list == 0) { list = new_list; new_list = 0; + /* Avoid infinite loop */ + if (++level >= EXT2FS_MAX_NESTED_LINKS) + break; } } free_dirlist(&list); diff --git a/lib/ext2fs/namei.c b/lib/ext2fs/namei.c index 6bbb124..a936474 100644 --- a/lib/ext2fs/namei.c +++ b/lib/ext2fs/namei.c @@ -20,6 +20,7 @@ #include "ext2_fs.h" #include "ext2fs.h" +#include "ext2fsP.h" static errcode_t open_namei(ext2_filsys fs, ext2_ino_t root, ext2_ino_t base, const char *pathname, size_t pathlen, int follow, @@ -45,7 +46,7 @@ static errcode_t follow_link(ext2_filsys fs, ext2_ino_t root, ext2_ino_t dir, *res_inode = inode; return 0; } - if (link_count++ > 5) { + if (link_count++ >= EXT2FS_MAX_NESTED_LINKS) { return EXT2_ET_SYMLINK_LOOP; } /* FIXME-64: Actually, this is FIXME EXTENTS */ -- 1.7.1 > On Nov 3, 2011, at 10:47 AM, Eric Sandeen wrote: > >> My only concern would be that depth 5 isn't totally unreasonable in real life, and this causes it to silently stop searching, right? >> Would there be much harm in making the limit much higher, to be fairly sure that it has wandered off into the weeds? > Agreed, the kernel currently uses a limit of 8. And we should use a #define for this in lib/ext2fs/ext2fsP.h, and use it for both finddev.c and lib/ext2fs/namei.c. > > -- Ted > ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() 2011-11-04 12:50 ` Niu @ 2011-11-20 4:18 ` Ted Ts'o 2011-11-20 8:17 ` Christian Kujau 1 sibling, 0 replies; 6+ messages in thread From: Ted Ts'o @ 2011-11-20 4:18 UTC (permalink / raw) To: Niu; +Cc: Eric Sandeen, linux-ext4 On Fri, Nov 04, 2011 at 08:50:38PM +0800, Niu wrote: > From 81bfd58b3980f940c23f87f891365a289df776ec Mon Sep 17 00:00:00 2001 > From: Niu Yawei <niu@whamcloud.com> > Date: Wed, 2 Nov 2011 04:31:11 +0800 > Subject: [PATCH] e2fsprogs: maximum nested link count > > Define EXT2FS_MAX_NESTED_LINKS as 8, and check the link count > not exceeding it in ext2fs_find_block_device() and follow_link(). > > Signed-off-by: Niu Yawei <niu@whamcloud.com> Thanks, applied. - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() 2011-11-04 12:50 ` Niu 2011-11-20 4:18 ` Ted Ts'o @ 2011-11-20 8:17 ` Christian Kujau 1 sibling, 0 replies; 6+ messages in thread From: Christian Kujau @ 2011-11-20 8:17 UTC (permalink / raw) To: Niu; +Cc: Theodore Tso, Eric Sandeen, linux-ext4 On Fri, 4 Nov 2011 at 20:50, Niu wrote: > diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h > index b182d7f..82e1ba0 100644 > --- a/lib/ext2fs/ext2fsP.h > +++ b/lib/ext2fs/ext2fsP.h > @@ -11,6 +11,8 @@ > > #include "ext2fs.h" > [...] > --- a/lib/ext2fs/finddev.c > +++ b/lib/ext2fs/finddev.c > @@ -34,6 +34,7 @@ > > #include "ext2_fs.h" > #include "ext2fs.h" > +#include "ext2fsP.h" On the very real chance of making a fool of myself: if ext2fsP.h includes ext2fs.h and now finddev.c will include ext2fsP.h - shouldn't we omit the '#include "ext2fs.h"' in finddev.c (and namei.c) now? I guess the compiler/preprocessor will figure this out, just wondering about code cleanliness. Then again we have names like 'ext2_fs.h' and 'ext2fs.h', I guess only an insider will know the difference :-) Christian. -- BOFH excuse #97: Small animal kamikaze attack on power supplies ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-20 8:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-03 6:45 e2fsprogs: Avoid infinite loop in ext2fs_find_block_device() Niu 2011-11-03 14:47 ` Eric Sandeen 2011-11-03 15:27 ` Theodore Tso 2011-11-04 12:50 ` Niu 2011-11-20 4:18 ` Ted Ts'o 2011-11-20 8:17 ` Christian Kujau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).