linux-kernel - Re: [PATCH] nilfs2: add ratelimiting to nilfs2 message

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAKFNMoms=mCn60bPy-L9L4bn+RozQW0urvDzPwW4Fgq0rH2HfA@mail.gmail.com>
Date: Tue, 1 Oct 2024 00:45:01 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: syzbot+8a192e8d090fa9a31135@...kaller.appspotmail.com, 
	linux-kernel@...r.kernel.org, linux-nilfs@...r.kernel.org, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] nilfs2: add ratelimiting to nilfs2 message

On Sat, Sep 28, 2024 at 3:18 AM Ryusuke Konishi wrote:
>
> On Sat, Sep 28, 2024 at 12:19 AM Lizhi Xu wrote:
> >
> > Syzbot report a task hung in vcs_open.
> > When rec_len too small in nilfs_check_folio, it can result in a huge flood
> > of messages being sent to the console. It eventually caused tty to hung when
> > retrieving the console_lock().
> >
> > Reported-by: syzbot+8a192e8d090fa9a31135@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=8a192e8d090fa9a31135
> > Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
> > ---
> >  fs/nilfs2/dir.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
>
> Thank you for the patch.
>
> I could confirm that the problem is reproducible and that your patch
> prevents it, so I will treat this as a nilfs2 side issue.
>
> The patch seems somewhat straightforward, so let me review this a bit
> more.  I may ask you to make some changes.
>
> Thanks,
> Ryusuke Konishi

Hi Lizhi,

I found that the root cause of this problem is that nilfs_find_entry()
does not abort the search loop even when nilfs_get_folio() returns an
error.

If the i_size of the directory inode is large and the directory is
corrupted, nilfs_find_entry() may continue to loop and output error
messages endlessly in bursts.

Rate-limiting may be able to prevent serial hangs, but it cannot
interrupt the near-endless loop in nilfs_find_entry(), so I don't
think it is the right approach to take to fix the problem.

Like ext2, nilfs_find_entry() should be able to return errors from
nilfs_get_folio() like this:

    char *kaddr = nilfs_get_folio(dir, n, foliop);

    if (IS_ERR(kaddr))
            return ERR_CAST(kaddr);

However, this approach requires some preparation changes so that the
error code returned by nilfs_find_entry() can be propagated to its
callers.

So, would you mind letting me fix this?

Or, if you want to do it yourself, please let me know.
If you refer to the ext2 implementation, you should be able to figure
out how to fix it, even though there are some nilfs-specific
differences.

Thanks,
Ryusuke Konishi

>
> >
> > diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> > index fe5b1a30c509..0a89dda75414 100644
> > --- a/fs/nilfs2/dir.c
> > +++ b/fs/nilfs2/dir.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/pagemap.h>
> >  #include "nilfs.h"
> >  #include "page.h"
> > +#include <linux/ratelimit.h>
> >
> >  static inline unsigned int nilfs_rec_len_from_disk(__le16 dlen)
> >  {
> > @@ -115,6 +116,7 @@ static bool nilfs_check_folio(struct folio *folio, char *kaddr)
> >         size_t limit = folio_size(folio);
> >         struct nilfs_dir_entry *p;
> >         char *error;
> > +       static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL * 5, 1);
> >
> >         if (dir->i_size < folio_pos(folio) + limit) {
> >                 limit = dir->i_size - folio_pos(folio);
> > @@ -148,9 +150,11 @@ static bool nilfs_check_folio(struct folio *folio, char *kaddr)
> >         /* Too bad, we had an error */
> >
> >  Ebadsize:
> > -       nilfs_error(sb,
> > -                   "size of directory #%lu is not a multiple of chunk size",
> > -                   dir->i_ino);
> > +       if (__ratelimit(&rs)) {
> > +               nilfs_error(sb,
> > +                           "size of directory #%lu is not a multiple of chunk size",
> > +                           dir->i_ino);
> > +       }
> >         goto fail;
> >  Eshort:
> >         error = "rec_len is smaller than minimal";
> > @@ -167,18 +171,22 @@ static bool nilfs_check_folio(struct folio *folio, char *kaddr)
> >  Einumber:
> >         error = "disallowed inode number";
> >  bad_entry:
> > -       nilfs_error(sb,
> > +       if (__ratelimit(&rs)) {
> > +               nilfs_error(sb,
> >                     "bad entry in directory #%lu: %s - offset=%lu, inode=%lu, rec_len=%zd, name_len=%d",
> >                     dir->i_ino, error, (folio->index << PAGE_SHIFT) + offs,
> >                     (unsigned long)le64_to_cpu(p->inode),
> >                     rec_len, p->name_len);
> > +       }
> >         goto fail;
> >  Eend:
> >         p = (struct nilfs_dir_entry *)(kaddr + offs);
> > -       nilfs_error(sb,
> > -                   "entry in directory #%lu spans the page boundary offset=%lu, inode=%lu",
> > -                   dir->i_ino, (folio->index << PAGE_SHIFT) + offs,
> > -                   (unsigned long)le64_to_cpu(p->inode));
> > +       if (__ratelimit(&rs)) {
> > +               nilfs_error(sb,
> > +                           "entry in directory #%lu spans the page boundary offset=%lu, inode=%lu",
> > +                           dir->i_ino, (folio->index << PAGE_SHIFT) + offs,
> > +                           (unsigned long)le64_to_cpu(p->inode));
> > +       }
> >  fail:
> >         return false;
> >  }
> > --
> > 2.43.0
> >