linux-kernel - Re: [PATCH v2 08/17] s390/pci: Remove races against pte updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3594c115-541f-806a-ee33-e99a2d1d31e8@linux.ibm.com>
Date:   Wed, 21 Oct 2020 09:55:57 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     DRI Development <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>, kvm@...r.kernel.org,
        linux-mm@...ck.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-media@...r.kernel.org,
        linux-s390@...r.kernel.org,
        Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Dan Williams <dan.j.williams@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        John Hubbard <jhubbard@...dia.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2 08/17] s390/pci: Remove races against pte updates

Hi Daniel,

friendly ping. I haven't seen a new version of this patch series,
as I said I think your change for s390/pci is generally useful so
I'm curious, are you planning on sending a new version soon?
If you want you can also just sent this patch with the last few
nitpicks (primarily the mail address) fixed and I'll happily apply.

Best regards,
Niklas Schnelle

On 10/12/20 4:19 PM, Daniel Vetter wrote:
> On Mon, Oct 12, 2020 at 04:03:28PM +0200, Niklas Schnelle wrote:
... snip ....
>>> Cc: Jason Gunthorpe <jgg@...pe.ca>
>>> Cc: Dan Williams <dan.j.williams@...el.com>
>>> Cc: Kees Cook <keescook@...omium.org>
>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>> Cc: John Hubbard <jhubbard@...dia.com>
>>> Cc: Jérôme Glisse <jglisse@...hat.com>
>>> Cc: Jan Kara <jack@...e.cz>
>>> Cc: Dan Williams <dan.j.williams@...el.com>
>>
>> The above Cc: line for Dan Williams is a duplicate
>>
>>> Cc: linux-mm@...ck.org
>>> Cc: linux-arm-kernel@...ts.infradead.org
>>> Cc: linux-samsung-soc@...r.kernel.org
>>> Cc: linux-media@...r.kernel.org
>>> Cc: Niklas Schnelle <schnelle@...ux.ibm.com>
>>> Cc: Gerald Schaefer <gerald.schaefer@...ux.ibm.com>
>>> Cc: linux-s390@...r.kernel.org
>>> --
>>> v2: Move VM_IO | VM_PFNMAP checks around so they keep returning EINVAL
>>> like before (Gerard)
>>
>> I think the above should go before the CC/Signed-off/Reviewev block.
> 
> This is a per-subsystem bikeshed :-) drivers/gpu definitely wants it
> above, but most core subsystems want it below. I'll move it.
> 
>>> ---
>>>  arch/s390/pci/pci_mmio.c | 98 +++++++++++++++++++++++-----------------
>>>  1 file changed, 57 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
>>> index 401cf670a243..1a6adbc68ee8 100644
>>> --- a/arch/s390/pci/pci_mmio.c
>>> +++ b/arch/s390/pci/pci_mmio.c
>>> @@ -119,33 +119,15 @@ static inline int __memcpy_toio_inuser(void __iomem *dst,
>>>  	return rc;
>>>  }
>>>  
>>> -static long get_pfn(unsigned long user_addr, unsigned long access,
>>> -		    unsigned long *pfn)
>>> -{
>>> -	struct vm_area_struct *vma;
>>> -	long ret;
>>> -
>>> -	mmap_read_lock(current->mm);
>>> -	ret = -EINVAL;
>>> -	vma = find_vma(current->mm, user_addr);
>>> -	if (!vma)
>>> -		goto out;
>>> -	ret = -EACCES;
>>> -	if (!(vma->vm_flags & access))
>>> -		goto out;
>>> -	ret = follow_pfn(vma, user_addr, pfn);
>>> -out:
>>> -	mmap_read_unlock(current->mm);
>>> -	return ret;
>>> -}
>>> -
>>>  SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
>>>  		const void __user *, user_buffer, size_t, length)
>>>  {
>>>  	u8 local_buf[64];
>>>  	void __iomem *io_addr;
>>>  	void *buf;
>>> -	unsigned long pfn;
>>> +	struct vm_area_struct *vma;
>>> +	pte_t *ptep;
>>> +	spinlock_t *ptl;
>>
>> With checkpatch.pl --strict the above yields a complained
>> "CHECK: spinlock_t definition without comment" but I think
>> that's really okay since your commit description is very clear.
>> Same oin line 277.
> 
> I think this is a falls positive, checkpatch doesn't realize that
> SYSCALL_DEFINE3 is a function, not a structure. And in a structure I'd
> have added the kerneldoc or comment.
> 
> I'll fix up all the nits you've found for the next round. Thanks for
> taking a look.
> -Daniel
>