Re: [patch][rfc] 5/5: core remove PageReserved

[Nick Piggin]

William Lee Irwin III wrote:
> William Lee Irwin III wrote:
>
> > > As exciting as this is, !!(vma->vm_flags & VM_RESERVED) could trivially
> > > go into struct zap_details without excess args or diff.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > Actually, it isn't trivial. I thought of trying that.
>
>
> You're probably thinking of the zap_page_range() vs. unmap_page_range()
> topmost-level entrypoints as being the only places to set the flag in
> details. Unconditionally clobbering the field in details in
> zap_pud_range() should resolve any issues associated with that.
>
> This is obviously a style and/or diff-minimization issue.

I'm thinking of exit_mmap and unmap_vmas, that don't pass in a
details field at all, and all the tests for details being marked
unlikely. I ended up thinking it was less ugly this way.

> William Lee Irwin III wrote:
>
> > > The refcounting and PG_reserved activity in memmap_init_zone() is
> > > superfluous. bootmem.c does all the necessary accounting internally.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > Well not superfluous yet. It is kept around to support all the arch
> > code that still uses it (not much, mainly reserved memory reporting).
>
>
> That activity is outside memmap_init_zone(). Specifically, the page
> structures are not used for anything whatsoever before bootmem puts
> them into the buddy allocators. It is not particularly interesting
> or difficult to defer even the initialization to the precise point
> in time bootmem cares to perform buddy bitmap insertion. This goes
> for all fields of struct page. What's notable about PG_reserved and
> refcounts here is that memmap_init_zone() goes about flipping bits one
> way where free_all_bootmem_core() undoes all its work.

This patch doesn't care how it works, that would be for a later patch.

> William Lee Irwin III wrote:
>
> > > There is no error returned here to be handled by the caller.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > That's OK, the pte has been cleared. Nothing else we can do.
>
>
> This is called in the interior of a loop, which may be beneficial to
> terminate if this intended semantic is to be enforced. Furthermore, no
> error is propagated to the caller, which is not the desired effect in
> the stated error reporting scheme. So the code is inconsistent with
> explicitly stated intention.

No, the error reporting scheme says it doesn't handle any error,
that is all. What we have here in terms of behaviour is exactly
what used to happen, that is - do something saneish on error.
Changing behaviour would be outside the scope of this patch, but
be my guest.

> William Lee Irwin III wrote:
>
> > > This has no effect but to artificially constrain the interface. There
> > > is no a priori reason to avoid the use of install_page() to deposit
> > > mappings to normal pages in VM_RESERVED vmas. It's only the reverse
> > > being "banned" here. Similar comments also apply to zap_pte().
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > No, install_page is playing with the page (eg. page_add_file_rmap)
> > which is explicity banned even before my PageReserved removal. It
> > is unclear that this ever safely worked for normal pages, and will
> > hit NULL page dereferences if trying to do it with iomem.
>
>
> You are going on about the fact that install_page() can't be used on
> memory outside mem_map[] as it requires a page structure, and can't be
> used on reserved pages because page_add_file_rmap() will BUG. This case
> is not being discussed.

And that it isn't allowed to touch struct page of physical pages
in a VM_RESERVED region.

> The issue at stake is inserting normal pages into a VM_RESERVED vma.
> These will arise as e.g. kernel-allocated buffers managed by normal
> reference counting. remap_pfn_range() can't do it; it refuses to
> operate on "valid memory". install_page() now won't do it; it refuses
> to touch a VM_RESERVED vma. So this creates a giant semantic hole,
> and potentially breaks working code (i.e. if you were going to do
> this you would need not only a replacement but also a sweep to adjust
> all the drivers doing it or prove their nonexistence).

I think you'll find that remap_pfn_range will be happy to operate
on valid memory, and that any driver trying to use install_page
on VM_RESERVED probably needs fixing anyway.

> William Lee Irwin III wrote:
>
> > > An answer should be devised for this. My numerous SCSI CD-ROM devices
> > > (I have 5 across several different machines of several different arches)
> > > are rather unlikely to be happy with /* FIXME: XXX ... as an answer.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > The worst it will do is dirty a VM_RESERVED page. So it is going
> > to work unless you're thinking about doing something crazy like
> > mmap /dev/mem and send your CDROM some pages from there. But yeah,
> > I have to find someone who knows what they're doing to look at this.
>
>
> Then you should replace FIXME: XXX with this explanation. By and large
> the presence of "FIXME: XXX" is a sign there is a gigantic hole in the
> code. It should definitely not be done with new code, but rather,
> exclusively confined to documenting discoveries of preexisting brokenness.
> After all, if a patch is introducing broken code, why would we merge it?
> Best to adjust the commentary and avoid that question altogether.

We wouldn't merge it. Hence this is an rfc and I explicitly said
it is not for merging.

> There are actually larger questions about this than the reserved page
> handling. If e.g. pagecache pages need to be dirtied the raw bitflag
> toggling is probably not how it should be done.

Yep.

> William Lee Irwin III wrote:
>
> > > snd_malloc_pages() marks all pages it allocates PG_reserved. This
> > > merits some commentary, and likely the removal of the superfluous
> > > PG_reserved usage.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > Sure, but not in this patch. The aim here is just to eliminate special
> > casing of refcounting. Other PG_reserved usage can stay around for the
> > moment (and is actually good for catching errors).
>
>
> Unfortunately for this scheme, it's very much a case of putting the
> cart before the horse. PG_reserved is toggled at random in this driver
> after this change, to no useful effect (debugging or otherwise). And
> this really goes for the whole affair. Diddling the core first is just
> going to create bugs. Converting the users first is the way these
> things need to be done. When complete, nothing needs the core flags
> twiddling anymore and you just nuke the flag twiddling from the core.

I'm sorry, I don't see how 'diddling' the core will create bugs.

This is a fine way to do it, and "converting" users first (whatever
that means) is not possible because VM_RESERVED handling in core
code is not up to the task of replacing PageReserved without this
patch.

> William Lee Irwin III wrote:
>
> > > This is user-triggerable where the driver honors mmap() protection
> > > flags blindly.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > If the user is allowed write access to VM_RESERVED memory, then I
> > suspect there is a lot worse they can do than flood the log.
> > But the check isn't going to stay around forever.
>
>
> This doesn't really do a whole lot of good for the unwitting user
> who invokes a privileged application relying on such kernel behavior.
> User-visible changes need to be taken on with somewhat more care
> (okay, vastly more, along with long-term backward compatibility).

It does a great lot of good, because they can tell us about it
we'll fix it. Search the kernel sources and you'll find other
examples that look almost exactly like this one.

> William Lee Irwin III wrote:
>
> > > This behavioral change needs to be commented on. There are some additional
> > > difficulties when memory holes are unintentionally covered by mem_map[];
> > > It is beneficial otherwise. It's likely to triplefault on such holes.
>
>
> On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:
>
> > It seems the author of this code themselves didn't really understand
> > what was going on here, so I'm buggered if I can be bothered :)
> > Remember though, PageReserved can stay around for as long as we need,
> > so this hunk can be trivially reverted if it is an immediate problem.
>
>
> This doesn't really fly. A fair number of drivers are poorly-understood
> and numerous gyrations have to be gone through to avoid breaking their
> possible assumptions until at long last clarifications are made (that
> is, if they're ever made). swsusp is not demotable to below their
> status on a whim.

Yep, that's why I'm going to ask some swsusp developers to have
a look at it.

I wouldn't pretend to be able to fix every bug everywhere in the
kernel myself.

Thanks,
Nick

--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/