linux-kernel - Re: [PATCH 05/11] afs: Fix to take ref on page when PG

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201028142041.GZ20115@casper.infradead.org>
Date:   Wed, 28 Oct 2020 14:20:41 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     David Howells <dhowells@...hat.com>
Cc:     linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/11] afs: Fix to take ref on page when PG_private is set

On Wed, Oct 28, 2020 at 02:10:24PM +0000, David Howells wrote:
> +++ b/fs/afs/dir.c
> @@ -283,6 +283,7 @@ static struct afs_read *afs_read_dir(struct afs_vnode *dvnode, struct key *key)
>  
>  			set_page_private(req->pages[i], 1);
>  			SetPagePrivate(req->pages[i]);
> +			get_page(req->pages[i]);

Alternative spelling:

-			set_page_private(req->pages[i], 1);
-			SetPagePrivate(req->pages[i]);
+			attach_page_private(req->pages[i], (void *)1);

AFS is an anomaly; most filesystems actually stick a pointer in page->private.

> +++ b/fs/afs/write.c
> @@ -151,7 +151,8 @@ int afs_write_begin(struct file *file, struct address_space *mapping,
>  	priv |= f;
>  	trace_afs_page_dirty(vnode, tracepoint_string("begin"),
>  			     page->index, priv);
> -	SetPagePrivate(page);
> +	if (!TestSetPagePrivate(page))
> +		get_page(page);
>  	set_page_private(page, priv);
>  	_leave(" = 0");
>  	return 0;

There's an efficiency question here that I can't answer ... how often do
you call afs_write_begin() on a page which already has PagePrivate set?
It's fewer atomic ops to do:

	if (PagePrivate(page))
		set_page_private(page, priv);
	else
		attach_page_private(page, (void *)priv);

I have no objection to adding TestSetPagePrivate per se; I just don't
know if it's really what you want or not.