Re: [patch 10/12] [PATCH] kvm-s390: storage key interface

[Heiko Carstens]

On Fri, Dec 09, 2011 at 12:23:36PM +0100, Carsten Otte wrote:
> This patch introduces an interface to access the guest visible
> storage keys. It supports three operations that model the behavior
> that SSKE/ISKE/RRBE instructions would have if they were issued by
> the guest. These instructions are all documented in the z architecture
> principles of operation book.
> 
> Signed-off-by: Carsten Otte <co...@de.ibm.com>

[...]

> +static long kvm_s390_keyop(struct kvm_s390_keyop *kop)
> +{
> +     unsigned long addr = kop->user_addr;
> +     pte_t *ptep;
> +     pgste_t pgste;
> +     int r;
> +     unsigned long skey;
> +     unsigned long bits;
> +
> +     /* make sure this process is a hypervisor */
> +     r = -EINVAL;
> +     if (!mm_has_pgste(current->mm))
> +             goto out;
> +
> +     r = -ENXIO;
> +     if (addr >= PGDIR_SIZE)
> +             goto out;

imho this should be -EFAULT.

> +     spin_lock(&current->mm->page_table_lock);
> +     ptep = ptep_for_addr(addr);
> +     if (!ptep)
> +             goto out_unlock;

ptep is a pointer and may contain an error code, like you implemented it
below. Therefore you need to check for IS_ERR() here.

> +static pmd_t *__pmdp_for_addr(struct mm_struct *mm, unsigned long addr)
> +{
> +     struct vm_area_struct *vma;
> +     pgd_t *pgd;
> +     pud_t *pud;
> +     pmd_t *pmd;
> +
> +     vma = find_vma(mm, addr);
> +     if (!vma)
> +             return ERR_PTR(-EINVAL);

-EFAULT imho.

Also, why is this check good enough? As far as I remember find_vma() only
guarantees that addr < vma_end, (if vma != NULL), but it does not guarantee
that addr >= vma_start.

> -             vma = find_vma(mm, vmaddr);
> -             if (!vma || vma->vm_start > vmaddr)
> -                     return -EFAULT;

... you used to check for that and also used the proper return code, btw.
Or is there a different reason why the above code is correct?

> +pte_t *ptep_for_addr(unsigned long addr)
> +{
> +     pmd_t *pmd;
> +     pte_t *rc;

Would you mind renaming rc into pte?

> +
> +     down_read(&current->mm->mmap_sem);
> +
> +     pmd = __pmdp_for_addr(current->mm, addr);
> +     if (IS_ERR(pmd)) {
> +             rc = (pte_t *)pmd;
> +             goto up_out;
> +     }
> +
> +     rc = pte_offset(pmd, addr);
> +up_out:
> +     up_read(&current->mm->mmap_sem);
> +     return rc;

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html