linux-kernel - Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace

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: <84338eac-c287-1826-4ac1-72cd17ee62cc@broadcom.com>
Date:   Fri, 30 Aug 2019 15:24:24 -0700
From:   James Smart <james.smart@...adcom.com>
To:     Sagi Grimberg <sagi@...mberg.me>, Christoph Hellwig <hch@....de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-nvme@...ts.infradead.org,
        Keith Busch <keith.busch@...el.com>,
        linux-kernel@...r.kernel.org, Hannes Reinecke <hare@...e.de>
Subject: Re: [PATCH v2 3/3] nvme: fire discovery log page change events to
 userspace

On 8/30/2019 2:07 PM, Sagi Grimberg wrote:
>
>>>>>> Yes we do, userspace should use it to order events.  Does udev not
>>>>>> handle that properly today?
>>>>>
>>>>> The problem is not ordering of events, its really about the fact that
>>>>> the chardev can be removed and reallocated for a different controller
>>>>> (could be a completely different discovery controller) by the time
>>>>> that userspace handles the event.
>>>>
>>>> The same is generally true for lot of kernel devices.  We could reduce
>>>> the chance by using the idr cyclic allocator.
>>>
>>> Well, it was raised by Hannes and James, so I'll ask them respond here
>>> because I don't mind having it this way. I personally think that this
>>> is a better approach than having a cyclic idr allocator. In general, I
>>> don't necessarily think that this is a good idea to have cyclic
>>> controller enumerations if we don't absolutely have to...
>>
>> We hit it right and left without the cyclic allocator, but that won't 
>> necessarily remove it.
>>
>> Perhaps we should have had a unique token assigned to the controller, 
>> and have the event pass the name and the token.  The cli would then, 
>> if the token is present, validate it via an ioctl before proceeding 
>> with other ioctls.
>>
>> Where all the connection arguments were added we due to the reuse 
>> issue and then solving the question of how to verify and/or lookup 
>> the desired controller, by using the shotgun approach rather than 
>> being very pointed, which is what the name/token would do.
>
> This unique token is: trtype:traddr:trsvcid:host-traddr ...

well yes :)  though rather verbose.   There is still a minute window as 
we're comparing values in sysfs, prior to opening the device, so 
technically something could change in that window between when we 
checked sysfs and when we open'd.   We can certain check after we open 
the device to solve that issue.

There is some elegance to a 32-bit token for the controller (can be an 
incrementing value) passed in the event and used when servicing the 
event that avoids a bunch of work.

Doing either of these would eliminate what Hannes liked - looking for 
the discovery controller with those attributes. Although, I don't know 
that looking for it is all that meaningful.