linux-kernel - Re: [PATCH] [14/21] HWPOISON: shmem: call set_page

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:	Fri, 11 Sep 2009 22:24:06 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	fengguang.wu@...el.com, linux-kernel@...r.kernel.org,
	"hugh.dickins" <hugh.dickins@...cali.co.uk>
Subject: Re: [PATCH] [14/21] HWPOISON: shmem: call set_page_dirty() with
 locked page

On Fri, 2009-09-11 at 20:48 +0200, Andi Kleen wrote:
> From: Wu Fengguang <fengguang.wu@...el.com>
> 
> The dirtying of page and set_page_dirty() can be moved into the page lock.
> 
> - In shmem_write_end(), the page was dirtied while the page lock was held,
>   but it's being marked dirty just after dropping the page lock.
> - In shmem_symlink(), both dirtying and marking can be moved into page lock.
> 
> It's valuable for the hwpoison code to know whether one bad page can be dropped
> without losing data. It mainly judges by testing the PG_dirty bit after taking
> the page lock. So it becomes important that the dirtying of page and the
> marking of dirtiness are both done inside the page lock. Which is a common
> practice, but sadly not a rule.
> 
> The noticeable exceptions are
> - mapped pages
> - pages with buffer_heads
> The above pages could go dirty at any time. Fortunately the hwpoison will
> unmap the page and release the buffer_heads beforehand anyway.
> 
> Many other types of pages (eg. metadata pages) can also be dirtied at will by
> their owners, the hwpoison code cannot do meaningful things to them anyway. 
> Only the dirtiness of pagecache pages owned by regular files are interested.

Its not clear to me what this patch achieves.

Will it, from now on, be mandatory to dirty pages under PG_lock, and
does this patch correct the last few such cases that didn't yet do that,
or does it still leave holes?

Can we document the requirements and or holes to it someplace near
set_page_dirty()?

> Acked-by: Hugh Dickins <hugh.dickins@...cali.co.uk>
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> Reviewed-by: WANG Cong <xiyou.wangcong@...il.com>
> Signed-off-by: Wu Fengguang <fengguang.wu@...el.com>
> ---
>  mm/shmem.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux/mm/shmem.c
> ===================================================================
> --- linux.orig/mm/shmem.c
> +++ linux/mm/shmem.c
> @@ -1630,8 +1630,8 @@ shmem_write_end(struct file *file, struc
>  	if (pos + copied > inode->i_size)
>  		i_size_write(inode, pos + copied);
>  
> -	unlock_page(page);
>  	set_page_dirty(page);
> +	unlock_page(page);
>  	page_cache_release(page);
>  
>  	return copied;
> @@ -1968,13 +1968,13 @@ static int shmem_symlink(struct inode *d
>  			iput(inode);
>  			return error;
>  		}
> -		unlock_page(page);
>  		inode->i_mapping->a_ops = &shmem_aops;
>  		inode->i_op = &shmem_symlink_inode_operations;
>  		kaddr = kmap_atomic(page, KM_USER0);
>  		memcpy(kaddr, symname, len);
>  		kunmap_atomic(kaddr, KM_USER0);
>  		set_page_dirty(page);
> +		unlock_page(page);
>  		page_cache_release(page);
>  	}
>  	if (dir->i_mode & S_ISGID)

--
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/