Re: [Patch] shmem_unuse race fix

[Christoph Rohland <cr@sap.com>]

Linus Torvalds <torvalds@transmeta.com> writes:

> On 28 Dec 2000, Christoph Rohland wrote:
> > 
> > First we need the following patch since otherwise we use a swap entry
> > without having the count increased:
> 
> No, that shouldn't be needed.
> 
> Look at the code-path: the kernel has the page locked, so nothing will
> de-allocate the swap entry - so it's perfectly ok to increase it
> later.

I am not sure that page locking is done very strictly in the swap
cache. I had to fiddle around that sometimes in shmem.c. 

The patch actually is getting me the 'right error': Undead swap
entries in swapoff instead of bad swap entries in nopage. The latter
means there is a swapentry noted in the page table which was legally
removed. And that's definitely wrong.

> I dislike the "magic" __get_swap_page(2) thing - we might make
> get_swap_page() itself _always_ return a swap entry with count two
> (one fot eh swap cache, one for the user), or we should keep it the
> way it was (where we explicitly increment it for the user).

Yes, I agree. I was too lazy to check the other uses of get_swap_page
for this patchlet. But at least shmem.c uses the same and I think it's
logical. We always need a swap cache and a user reference.

> Ok. How about making try_to_unuse() just get the VM semaphore instead of
> the page table lock?
> 
> Then try_to_unuse() would follow all the right rules, and the above
> problem wouldn't exist..

If this is right we should do this. There is no need to care about
contention in swapoff. It's definitely not the common path. But we
have to be careful about deadlocks....

Greetings
                Christoph

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/