Re: [PATCH] habanalabs: use correct variable to show fd open counter

[Oded Gabbay]

On Mon, Jul 8, 2019 at 2:21 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 08, 2019 at 01:43:55PM +0300, Oded Gabbay wrote:
> > The current code checks if the user context pointer is NULL or not to
> > display the number of open file descriptors of a device. However, that
> > variable (user_ctx) will eventually go away as the driver will support
> > multiple processes. Instead, the driver can use the atomic counter of
> > the open file descriptors which the driver already maintains.
> >
> > Signed-off-by: Oded Gabbay <oded.gabbay@xxxxxxxxx>
> > ---
> >  drivers/misc/habanalabs/sysfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/habanalabs/sysfs.c b/drivers/misc/habanalabs/sysfs.c
> > index 25eb46d29d88..881be19b5fad 100644
> > --- a/drivers/misc/habanalabs/sysfs.c
> > +++ b/drivers/misc/habanalabs/sysfs.c
> > @@ -356,7 +356,7 @@ static ssize_t write_open_cnt_show(struct device *dev,
> >  {
> >       struct hl_device *hdev = dev_get_drvdata(dev);
> >
> > -     return sprintf(buf, "%d\n", hdev->user_ctx ? 1 : 0);
> > +     return sprintf(buf, "%d\n", atomic_read(&hdev->fd_open_cnt));
> >  }
>
> Odds are, this means nothing, as it doesn't get touched if the file
> descriptor is duped or sent to another process.
>
> Why do you care about the number of open files?  Whenever someone tries
> to do this type of logic, it is almost always wrong, just let userspace
> do what it wants to do, and if wants to open something twice, then it
> gets to keep the pieces when things break.
>
> thanks,
>
> greg k-h

I care about the number of open file descriptors because I can't let
multiple processes run simultaneously on my device, as we still don't
have the code to do proper isolation between the processes, in regard
of memory accesses on our device memory and by using our DMA engine.
Basically, it's a security hole. If you want, I can explain more in
length on this issue.

We have the H/W infrastructure for that, using MMU and multiple
address space IDs (ASID), but we didn't write the code yet in the
driver, as that is a BIG feature but it wasn't requested by anyone
yet.

So the current solution is to block the ability to open multiple file
descriptors.

Regarding this specific sysfs property, I don't really care about it.
I simply saw that it is shown in other drivers and I thought it may be
nice for a system admin utility to show it.

Thanks,
Oded