netdev - Re: [PATCH RFC] vhost: don't use kmap() to log dirty pages

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: <20190507154753.GA8809@infradead.org>
Date:   Tue, 7 May 2019 08:47:53 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Jason Wang <jasowang@...hat.com>
Cc:     mst@...hat.com, kvm@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        James Bottomley <James.Bottomley@...senPartnership.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Darren Hart <dvhart@...radead.org>
Subject: Re: [PATCH RFC] vhost: don't use kmap() to log dirty pages

On Mon, May 06, 2019 at 10:23:29PM -0400, Jason Wang wrote:
> Note: there're archs (few non popular ones) that don't implement
> futex helper, we can't log dirty pages. We can fix them on top or
> simply disable LOG_ALL features of vhost.

That means vhost now has to depend on HAVE_FUTEX_CMPXCHG to make
sure we have a working implementation.


>  #include <linux/sched/signal.h>
>  #include <linux/interval_tree_generic.h>
>  #include <linux/nospec.h>
> +#include <asm/futex.h>

Also please include the futex maintainers to make sure they are fine
with this first usage of <asm/futex.h> outside of kernel/futex.c.


> +static int set_bit_to_user(int nr, u32 __user *addr)
>  {
>  	unsigned long log = (unsigned long)addr;
>  	struct page *page;
> +	u32 old_log;
>  	int r;
>  
>  	r = get_user_pages_fast(log, 1, 1, &page);
>  	if (r < 0)
>  		return r;
>  	BUG_ON(r != 1);
> +
> +	r = futex_atomic_cmpxchg_inatomic(&old_log, addr, 0, 0);
> +	if (r < 0)
> +		return r;
> +
> +	old_log |= 1 << nr;
> +	r = put_user(old_log, addr);
> +	if (r < 0)
> +		return r;

And this just looks odd to me.  Why do we need the futex call to
replace a 0 value with 0?  Why does it still duplicate the
put_user?  This doesn't look like actually working code to me.

Also don't we need a pagefault_disable() around
futex_atomic_cmpxchg_inatomic?