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

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

Details

Reviewers

alc
markj

Commits

rP462937: Add patch to enable on powerpcspe.
rP462840: Add support for powerpcspe arch.
rS329215: Do not leak rv->psind in some specific situations.

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib created this revision.Feb 12 2018, 7:48 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 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.

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.

In D14335#300497, @markj 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.

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.

In D14335#300520, @kib wrote:

In D14335#300497, @markj 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

Closed by commit rS329215: Do not leak rv->psind in some specific situations. (authored by kib). · Explain WhyFeb 13 2018, 3:36 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

head/

sys/

vm/

vm_reserv.c

1 line

Diff 39253

View Options

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