linux-kernel - Re: [PATCH 2/2] sysfs: only access bin file vm

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

Message-ID: <m1zkv9v3rg.fsf@fess.ebiederm.org>
Date:	Wed, 22 Sep 2010 14:59:15 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Hugh Dickins <hughd@...gle.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>, linux-kernel@...r.kernel.org,
	Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH 2/2] sysfs: only access bin file vm_ops with the active lock

Hugh Dickins <hughd@...gle.com> writes:

> On Mon, 20 Sep 2010, Eric W. Biederman wrote:
>> 
>> bb->vm_ops is a cached copy of the vm_ops of the underlying
>> sysfs bin file, which means that after sysfs_bin_remove_file
>> completes it is only longer valid to deference bb->vm_ops.
>> 
>> So move all of the tests of bb->vm_ops inside of where
>> we hold the sysfs active lock.
>> 
>> Signed-off-by: Eric W. Biederman <ebiederm@...stanetworks.com>
>
> Good point.  There's a couple of cases (the ones below) where you've
> changed the return value from success to failure when the entry point
> didn't exist for an object that is no longer active; but on reflection,
> I accept that's just not worth worrying about.

Good spotting and thanks for the review.

Thinking about this it looks like bin_set_policy should only ever
error if the underlying function errors, and that will clear up
one inconsistency.

As for bin_page_mkwrite if sysfs_get_active fails there is no mapping
because we have called unmap_mapping_range to zap everything, so
page_mkwrite should simply not be called.  However there is the
interesting case what should page_mkwrite return if there are no
vm_ops at all.

I expect the right choice for page_mkwrite it to simply have to
different sets of vm_ops and only write of page_mkwrite in one
of them and to simply call VM_FAULT_NOPAGE on error.  Implementing
page_mkwrite when it isn't needed appears to have all kinds of
interesting side effects where we do a lot more work than we other
wise would.  Lock dropping and regrabbing, as well as some extra
dirty page balancing logic gets triggered.

I will see if I can cook up a patch to that effect.

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