linux-kernel - Re: [PATCH] fs-writeback: fix NULL pointer dereference in __mark_inode

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20110317221925.GA4841@swordfish>
Date:	Fri, 18 Mar 2011 00:19:26 +0200
From:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Andreas Bießmann 
	<andreas.devel@...glemail.com>,
	"Jason A. Donenfeld" <Jason@...c4.com>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
	Anton Altaparmakov <aia21@...tab.net>,
	George Spelvin <linux@...izon.com>
Subject: Re: [PATCH] fs-writeback: fix NULL pointer dereference in
 __mark_inode_dirty


Hello,

On (03/17/11 14:04), Andrew Morton wrote:
>[..]
> afaik this regression didn't get fixed.  Jens put out a patch for
> George to test but there hasn't been any feedback on that yet.  Could
> you guys please give it a spin?
>


Sorry for rather long reply. Seem to work fine for me. 

Tested-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>


Thanks,
	Sergey

 
> From: Jens Axboe <axboe@...nel.dk>
> 
> When we move the potential dirty list entries to the
> default_backing_dev_info, reassign the sb->s_bdi as well. 
> default_backing_dev_info will always be around.  I hope this can fix it up
> for 2.6.38 and we can add the proper ref counting for .39.
> 
> Cc: Anton Altaparmakov <aia21@....ac.uk>
> Cc: George Spelvin <linux@...izon.com>
> Cc: Christoph Hellwig <hch@....de>
> Cc: Andreas Biemann <biessmann@...science.de>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> Tested-by: Torsten Hilbrich <torsten.hilbrich@...unet.com>
> Cc: <stable@...nel.org>		[2.6.38.x]
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> ---
> 
>  fs/super.c       |    2 ++
>  fs/sync.c        |    4 ++--
>  mm/backing-dev.c |    2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff -puN fs/super.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb fs/super.c
> --- a/fs/super.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb
> +++ a/fs/super.c
> @@ -72,6 +72,7 @@ static struct super_block *alloc_super(s
>  #else
>  		INIT_LIST_HEAD(&s->s_files);
>  #endif
> +		s->s_bdi = &default_backing_dev_info;
>  		INIT_LIST_HEAD(&s->s_instances);
>  		INIT_HLIST_BL_HEAD(&s->s_anon);
>  		INIT_LIST_HEAD(&s->s_inodes);
> @@ -1006,6 +1007,7 @@ vfs_kern_mount(struct file_system_type *
>  	}
>  	BUG_ON(!mnt->mnt_sb);
>  	WARN_ON(!mnt->mnt_sb->s_bdi);
> +	WARN_ON(mnt->mnt_sb->s_bdi == &default_backing_dev_info);
>  	mnt->mnt_sb->s_flags |= MS_BORN;
>  
>  	error = security_sb_kern_mount(mnt->mnt_sb, flags, secdata);
> diff -puN fs/sync.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb fs/sync.c
> --- a/fs/sync.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb
> +++ a/fs/sync.c
> @@ -33,7 +33,7 @@ static int __sync_filesystem(struct supe
>  	 * This should be safe, as we require bdi backing to actually
>  	 * write out data in the first place
>  	 */
> -	if (!sb->s_bdi || sb->s_bdi == &noop_backing_dev_info)
> +	if (sb->s_bdi == &noop_backing_dev_info)
>  		return 0;
>  
>  	if (sb->s_qcop && sb->s_qcop->quota_sync)
> @@ -79,7 +79,7 @@ EXPORT_SYMBOL_GPL(sync_filesystem);
>  
>  static void sync_one_sb(struct super_block *sb, void *arg)
>  {
> -	if (!(sb->s_flags & MS_RDONLY) && sb->s_bdi)
> +	if (!(sb->s_flags & MS_RDONLY))
>  		__sync_filesystem(sb, *(int *)arg);
>  }
>  /*
> diff -puN mm/backing-dev.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb mm/backing-dev.c
> --- a/mm/backing-dev.c~vfs-fix-null-pointer-oops-in-sync_inodes_sb
> +++ a/mm/backing-dev.c
> @@ -598,7 +598,7 @@ static void bdi_prune_sb(struct backing_
>  	spin_lock(&sb_lock);
>  	list_for_each_entry(sb, &super_blocks, s_list) {
>  		if (sb->s_bdi == bdi)
> -			sb->s_bdi = NULL;
> +			sb->s_bdi = &default_backing_dev_info;
>  	}
>  	spin_unlock(&sb_lock);
>  }
> _
> 
> 
> btw, Christoph: would this not have been be a less hacky hack?
> 
> --- a/fs/fs-writeback.c~a
> +++ a/fs/fs-writeback.c
> @@ -73,7 +73,7 @@ static inline struct backing_dev_info *i
>  {
>  	struct super_block *sb = inode->i_sb;
>  
> -	if (strcmp(sb->s_type->name, "bdev") == 0)
> +	if (sb == blockdev_superblock)
>  		return inode->i_mapping->backing_dev_info;
>  
>  	return sb->s_bdi;
> _
> 

Content of type "application/pgp-signature" skipped