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] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 20 Dec 2006 19:52:00 +0300
From:	"Tomasz Kvarsin" <kvarsin@...il.com>
To:	"Andrew Morton" <akpm@...l.org>,
	"Tomasz Kvarsin" <kvarsin@...il.com>,
	"Alexander Viro" <viro@...iv.linux.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [BUG] [PATCH] [RFC] garbage instead of zeroes in UFS

Works for me. At least now I can not reproduce this bug.

On 12/20/06, Evgeniy Dushistov <dushistov@...l.ru> wrote:
> On Wed, Dec 20, 2006 at 03:09:55AM -0800, Andrew Morton wrote:
> > On Wed, 20 Dec 2006 14:04:06 +0300
> > "Tomasz Kvarsin" <kvarsin@...il.com> wrote:
> >
> > > Forgot to say I use linux-2.6.20-rc1-mm1
> > >
> > > On 12/20/06, Tomasz Kvarsin <kvarsin@...il.com> wrote:
> > > > I have some problems with write support of UFS.
> > > > Here is script which demonstrate problem:
> > > >
> > > > #create image
> > > > mkdir /tmp/ufs-expirements && cd /tmp/ufs-expirements/
> > > > for ((i=0; i<1024*1024*2; ++i)); do printf "z"; done > image
> > > >
> > > > #build ufs tools
> > > > wget 'http://heanet.dl.sourceforge.net/sourceforge/ufs-linux/ufs-tools-0.1.tar.bz2'
> > > > && tar xjf ufs-tools-0.1.tar.bz2 && cd ufs-tools-0.1
> > > > wget http://lkml.org/lkml/diff/2006/5/20/48/1 -O build.patch
> > > > patch -p1 < build.patch && make
> > > >
> > > > #create UFS file system on image
> > > > ./mkufs -O 1 -b 16384 -f 2048 ../image
> > > > cd .. && mkdir root
> > > > mount -t ufs image root -o loop,ufstype=44bsd
> > > > cd root/
> > > > touch a.txt
> > > > echo "END" > end.txt
> > > > dd if=./end.txt of=./a.txt bs=16384 seek=1
> > > >
> > > > and at the end content of "a.txt" not only  "END" and zeroes,
> > > > "a.txt" also contains "z".
> > > >
> > > > The real situation happened when I deleted big file,
> > > > and create new one with holes. This script just easy way to reproduce bug.
> > > >
> >
> > Does 2.6.20-rc1 have the same problem?
>
> Looks like this is the problem, which point Al Viro some time ago:
> when we allocate block(in this situation 16K) we mark as new
> only one fragment(in this situation 2K),
> I don't see _right_ way to do nullification of whole block,
> if use inode page cache, some pages may be outside of inode limits
> (inode size),
> and will be lost;
> if use blockdev page cache it is possible to zeroize real data,
> if later inode page cache will be used.
>
> The simpliest way, as can I see usage of block device page cache,
> but not only mark dirty, but also sync it during "nullification".
>
> Patch bellow properly handle Tomasz's test case for me(Tomasz can you try it?),
> but I am not sure is this _right_ solution.
> Any ideas?
>
> ---
>
> Index: linux-2.6.20-rc1-mm1/fs/ufs/inode.c
> ===================================================================
> --- linux-2.6.20-rc1-mm1.orig/fs/ufs/inode.c
> +++ linux-2.6.20-rc1-mm1/fs/ufs/inode.c
> @@ -156,36 +156,6 @@ out:
>         return ret;
>  }
>
> -static void ufs_clear_frag(struct inode *inode, struct buffer_head *bh)
> -{
> -       lock_buffer(bh);
> -       memset(bh->b_data, 0, inode->i_sb->s_blocksize);
> -       set_buffer_uptodate(bh);
> -       mark_buffer_dirty(bh);
> -       unlock_buffer(bh);
> -       if (IS_SYNC(inode))
> -               sync_dirty_buffer(bh);
> -}
> -
> -static struct buffer_head *
> -ufs_clear_frags(struct inode *inode, sector_t beg,
> -               unsigned int n, sector_t want)
> -{
> -       struct buffer_head *res = NULL, *bh;
> -       sector_t end = beg + n;
> -
> -       for (; beg < end; ++beg) {
> -               bh = sb_getblk(inode->i_sb, beg);
> -               ufs_clear_frag(inode, bh);
> -               if (want != beg)
> -                       brelse(bh);
> -               else
> -                       res = bh;
> -       }
> -       BUG_ON(!res);
> -       return res;
> -}
> -
>  /**
>   * ufs_inode_getfrag() - allocate new fragment(s)
>   * @inode - pointer to inode
> @@ -302,7 +272,7 @@ repeat:
>         }
>
>         if (!phys) {
> -               result = ufs_clear_frags(inode, tmp, required, tmp + blockoff);
> +               result = sb_getblk(sb, tmp + blockoff);
>         } else {
>                 *phys = tmp + blockoff;
>                 result = NULL;
> @@ -403,8 +373,7 @@ repeat:
>
>
>         if (!phys) {
> -               result = ufs_clear_frags(inode, tmp, uspi->s_fpb,
> -                                        tmp + blockoff);
> +               result = sb_getblk(sb, tmp + blockoff);
>         } else {
>                 *phys = tmp + blockoff;
>                 *new = 1;
> @@ -471,13 +440,13 @@ int ufs_getfrag_block(struct inode *inod
>  #define GET_INODE_DATABLOCK(x) \
>         ufs_inode_getfrag(inode, x, fragment, 1, &err, &phys, &new, bh_result->b_page)
>  #define GET_INODE_PTR(x) \
> -       ufs_inode_getfrag(inode, x, fragment, uspi->s_fpb, &err, NULL, NULL, bh_result->b_page)
> +       ufs_inode_getfrag(inode, x, fragment, uspi->s_fpb, &err, NULL, NULL, NULL)
>  #define GET_INDIRECT_DATABLOCK(x) \
>         ufs_inode_getblock(inode, bh, x, fragment,      \
> -                         &err, &phys, &new, bh_result->b_page);
> +                         &err, &phys, &new, bh_result->b_page)
>  #define GET_INDIRECT_PTR(x) \
>         ufs_inode_getblock(inode, bh, x, fragment,      \
> -                         &err, NULL, NULL, bh_result->b_page);
> +                         &err, NULL, NULL, NULL)
>
>         if (ptr < UFS_NDIR_FRAGMENT) {
>                 bh = GET_INODE_DATABLOCK(ptr);
> Index: linux-2.6.20-rc1-mm1/fs/ufs/balloc.c
> ===================================================================
> --- linux-2.6.20-rc1-mm1.orig/fs/ufs/balloc.c
> +++ linux-2.6.20-rc1-mm1/fs/ufs/balloc.c
> @@ -275,6 +275,25 @@ static void ufs_change_blocknr(struct in
>         UFSD("EXIT\n");
>  }
>
> +static void ufs_clear_frags(struct inode *inode, sector_t beg, unsigned int n,
> +                           int sync)
> +{
> +       struct buffer_head *bh;
> +       sector_t end = beg + n;
> +
> +       for (; beg < end; ++beg) {
> +               bh = sb_getblk(inode->i_sb, beg);
> +               lock_buffer(bh);
> +               memset(bh->b_data, 0, inode->i_sb->s_blocksize);
> +               set_buffer_uptodate(bh);
> +               mark_buffer_dirty(bh);
> +               unlock_buffer(bh);
> +               if (IS_SYNC(inode) || sync)
> +                       sync_dirty_buffer(bh);
> +               brelse(bh);
> +       }
> +}
> +
>  unsigned ufs_new_fragments(struct inode * inode, __fs32 * p, unsigned fragment,
>                            unsigned goal, unsigned count, int * err, struct page *locked_page)
>  {
> @@ -350,6 +369,8 @@ unsigned ufs_new_fragments(struct inode
>                         *p = cpu_to_fs32(sb, result);
>                         *err = 0;
>                         UFS_I(inode)->i_lastfrag = max_t(u32, UFS_I(inode)->i_lastfrag, fragment + count);
> +                       ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
> +                                       locked_page != NULL);
>                 }
>                 unlock_super(sb);
>                 UFSD("EXIT, result %u\n", result);
> @@ -363,6 +384,8 @@ unsigned ufs_new_fragments(struct inode
>         if (result) {
>                 *err = 0;
>                 UFS_I(inode)->i_lastfrag = max_t(u32, UFS_I(inode)->i_lastfrag, fragment + count);
> +               ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
> +                               locked_page != NULL);
>                 unlock_super(sb);
>                 UFSD("EXIT, result %u\n", result);
>                 return result;
> @@ -398,6 +421,8 @@ unsigned ufs_new_fragments(struct inode
>                 *p = cpu_to_fs32(sb, result);
>                 *err = 0;
>                 UFS_I(inode)->i_lastfrag = max_t(u32, UFS_I(inode)->i_lastfrag, fragment + count);
> +               ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
> +                               locked_page != NULL);
>                 unlock_super(sb);
>                 if (newcount < request)
>                         ufs_free_fragments (inode, result + newcount, request - newcount);
>
>
> --
> /Evgeniy
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists