Page MenuHomeFreeBSD

Do not leak rv->psind in some weird situations.
ClosedPublic

Authored by kib on Feb 12 2018, 7:48 PM.

Details

Summary

Suppose that we have an object with a mapped superpage, and that all pages in the superpages are held (by some driver). Then, suppose that the object is terminated, e.g. because the only process that mapped it is exiting . Then, the reservation is broken, but the pages cannot be freed until later, when they are unheld. In this situation, the reservation code cannot clean psind, since no pages are freed, and the page is freed and then reused with invalid psind.

Clean psind on vm_reserv_break() to avoid the situation.

BTW, I do not see any called for vm_reserv_break() which passes non-NULL m.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Feb 12 2018, 7:48 PM
markj added inline comments.Feb 12 2018, 9:06 PM
sys/vm/vm_reserv.c
952 ↗(On Diff #39215)

Hmm, I think this is insufficient. In the case you described, won't we still leave the bitmap and popcnt field set to the previous values? I'm not sure whether that will cause real problems when the reservation is reused, but it does look like it'll cause assertion failures.

markj added a comment.Feb 12 2018, 9:10 PM

BTW, I do not see any called for vm_reserv_break() which passes non-NULL m.

The last such caller went away in r308691, when PG_CACHED was removed.

kib added a comment.Feb 12 2018, 10:09 PM

BTW, I do not see any called for vm_reserv_break() which passes non-NULL m.

The last such caller went away in r308691, when PG_CACHED was removed.

Yes, I will clear this by removing the arg and if (m != NULL) code after this patch is reviewed. Just wanted to get a nod in advance.

sys/vm/vm_reserv.c
952 ↗(On Diff #39215)

I do not think that this is what done at the vm_reserv_break(). The function ensures that the rv->popcnt is zero and population map is empty, see the assert at the very end of the function. But break can only return the pages not marked as used in the population bitmap.

If any page from the reservation can be freed during the object termination, then the vm_object_terminate_pages() call before vm_reserv_break() call would already clear psind. It is very contrived situation when all superpage' pages are held.

markj accepted this revision.Feb 12 2018, 10:23 PM
In D14335#300520, @kib wrote:

BTW, I do not see any called for vm_reserv_break() which passes non-NULL m.

The last such caller went away in r308691, when PG_CACHED was removed.

Yes, I will clear this by removing the arg and if (m != NULL) code after this patch is reviewed. Just wanted to get a nod in advance.

Sounds good.

sys/vm/vm_reserv.c
952 ↗(On Diff #39215)

Ah, I didn't reread vm_reserv_break() carefully enough.

This revision is now accepted and ready to land.Feb 12 2018, 10:23 PM
This revision was automatically updated to reflect the committed changes.