Messages in this thread Patch in this message | | | Date | Thu, 23 Nov 2023 10:58:20 +0000 | From | Sebastian Ene <> | Subject | Re: [PATCH v3 10/10] arm64: ptdump: Add support for guest stage-2 pagetables dumping |
| |
On Wed, Nov 22, 2023 at 11:35:57PM +0000, Oliver Upton wrote: > On Wed, Nov 15, 2023 at 05:16:40PM +0000, Sebastian Ene wrote: > > +struct ptdump_registered_guest { > > + struct list_head reg_list; > > + struct ptdump_info info; > > + struct kvm_pgtable_snapshot snapshot; > > + rwlock_t *lock; > > +}; > > Why can't we just store a pointer directly to struct kvm in ::private?
I don't think it will work unless we expect a struct kvm_pgtable in stage2_ptdump_walk file_priv field. I think it is a good ideea and will simplify things a little bit dropping kvm_pgtable_snapshot from here.
The current code has some fileds that are reduntant (the priv pointers) because I also made this to work with protected guests where we can't access their pagetables directly.
> Also, shouldn't you take a reference on struct kvm when the file is > opened to protect against VM teardown? >
I am not sure about the need could you please elaborate a bit ? On VM teardown we expect ptdump_unregister_guest_stage2 to be invoked.
> > +static LIST_HEAD(ptdump_guest_list); > > +static DEFINE_MUTEX(ptdump_list_lock); > > What is the list for? >
I am keeping a list of registered guests with ptdump and the lock should protect the list against concurent VM teardowns.
> > static phys_addr_t ptdump_host_pa(void *addr) > > { > > return __pa(addr); > > @@ -757,6 +768,63 @@ static void stage2_ptdump_walk(struct seq_file *s, struct ptdump_info *info) > > kvm_pgtable_walk(pgtable, start_ipa, end_ipa, &walker); > > } > > > > +static void guest_stage2_ptdump_walk(struct seq_file *s, > > + struct ptdump_info *info) > > +{ > > + struct ptdump_info_file_priv *f_priv = > > + container_of(info, struct ptdump_info_file_priv, info); > > + struct ptdump_registered_guest *guest = info->priv; > > + > > + f_priv->file_priv = &guest->snapshot; > > + > > + read_lock(guest->lock); > > + stage2_ptdump_walk(s, info); > > + read_unlock(guest->lock); > > Taking the mmu lock for read allows other table walkers to add new > mappings and adjust the granularity of existing ones. Should this > instead take the mmu lock for write? >
Thanks for pointing our, this is exactly what I was trying to avoid, so yes I should use the write mmu lock in this case.
> > +} > > + > > +int ptdump_register_guest_stage2(struct kvm *kvm) > > +{ > > + struct ptdump_registered_guest *guest; > > + struct kvm_s2_mmu *mmu = &kvm->arch.mmu; > > + struct kvm_pgtable *pgt = mmu->pgt; > > + > > + guest = kzalloc(sizeof(struct ptdump_registered_guest), GFP_KERNEL); > > You want GFP_KERNEL_ACCOUNT here. >
Right, thanks this is because it is an untrusted allocation triggered from userspace.
> -- > Thanks, > Oliver
Thank you, Seb
| |