linux-kernel - Re: [UPDATE PATCH] mm: shmem: don't truncate page if memory failure happens

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]

Message-ID: <YZErCLzSixIwTr6C@casper.infradead.org>
Date:   Sun, 14 Nov 2021 15:28:08 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Yang Shi <shy828301@...il.com>
Cc:     torvalds@...ux-foundation.org, arnd@...db.de, hughd@...gle.com,
        kirill.shutemov@...ux.intel.com, naoya.horiguchi@....com,
        osalvador@...e.de, peterx@...hat.com, ajaygargnsit@...il.com,
        songmuchun@...edance.com, akpm@...ux-foundation.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [UPDATE PATCH] mm: shmem: don't truncate page if memory failure
 happens

On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote:
> @@ -2466,7 +2467,18 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
>  			return -EPERM;
>  	}
>  
> -	return shmem_getpage(inode, index, pagep, SGP_WRITE);
> +	ret = shmem_getpage(inode, index, pagep, SGP_WRITE);
> +
> +	if (ret)
> +		return ret;
> +
> +	if (*pagep && PageHWPoison(*pagep)) {
> +		unlock_page(*pagep);
> +		put_page(*pagep);
> +		ret = -EIO;

You definitely need to add:

		*pagep = NULL;

I'm not entirely convinced that you need the conditional on '*pagep'.
If we returned 0, there had better be a page at pagep!

I also think this would be clearer if written as:

	if (PageHWPoison(*pagep)) {
		unlock_page(*pagep);
		put_page(*pagep);
		*pagep = NULL;
		return -EIO;
	}

	return 0;

instead of re-using ret.  Sometimes that can make the code flow clearer,
but here, I don't think it does.

> @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping,
>  	error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE,
>  				  gfp, NULL, NULL, NULL);
>  	if (error)
> -		page = ERR_PTR(error);
> -	else
> -		unlock_page(page);
> +		return ERR_PTR(error);
> +
> +	unlock_page(page);
> +	if (PageHWPoison(page))
> +		return ERR_PTR(-EIO);

Do we need to put_page() the page in this error case?