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 <cotte@xxxxxxxxxx>
[...]
> +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(¤t->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(¤t->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(¤t->mm->mmap_sem);
> + return rc;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
