linux-kernel - Re: [PATCH 20/21] binder: reverse locking order in shrinker callback

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: <20231107090846.261721-1-aliceryhl@google.com>
Date:   Tue,  7 Nov 2023 09:08:46 +0000
From:   Alice Ryhl <aliceryhl@...gle.com>
To:     Carlos Llamas <cmllamas@...gle.com>
Cc:     "Arve Hjønnevåg" <arve@...roid.com>,
        Christian Brauner <brauner@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        kernel-team@...roid.com, linux-kernel@...r.kernel.org,
        Martijn Coenen <maco@...roid.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Todd Kjos <tkjos@...roid.com>
Subject: Re: [PATCH 20/21] binder: reverse locking order in shrinker callback

Carlos Llamas <cmllamas@...gle.com> writes:
> +	trace_binder_unmap_kernel_start(alloc, index);
> +
> +	__free_page(page->page_ptr);
> +	page->page_ptr = NULL;
> +
> +	trace_binder_unmap_kernel_end(alloc, index);
> +
>  	list_lru_isolate(lru, item);
> +	mutex_unlock(&alloc->mutex);
>  	spin_unlock(lock);
>  
>  	if (vma) {
>  		trace_binder_unmap_user_start(alloc, index);
>  
>  		zap_page_range_single(vma, page_addr, PAGE_SIZE, NULL);
>  
>  		trace_binder_unmap_user_end(alloc, index);
>  	}
> +
>  	mmap_read_unlock(mm);
>  	mmput_async(mm);
>  
> -	trace_binder_unmap_kernel_start(alloc, index);
> -
> -	__free_page(page->page_ptr);
> -	page->page_ptr = NULL;
> -
> -	trace_binder_unmap_kernel_end(alloc, index);

This reverses the order so that __free_page is called before
zap_page_range_single. Is that okay?

Another option would be to do something along these lines:

    page_to_free = page->page_ptr;
    page->page_ptr = NULL;

    [snip]

    mmap_read_unlock(mm);
    mmput_async(mm);
    __free_page(page_to_free);

This way you wouldn't reverse the order. Also, you reduce the amount of
time you spend holding the mmap read lock, since the page is freed
without holding the lock.

Other than the above, this LGTM.

Alice