netdev - Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event

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: <20190625202320.GJ3607@mellanox.com>
Date:   Tue, 25 Jun 2019 20:23:24 +0000
From:   Jason Gunthorpe <jgg@...lanox.com>
To:     Yishai Hadas <yishaih@....mellanox.co.il>
CC:     Leon Romanovsky <leon@...nel.org>,
        Doug Ledford <dledford@...hat.com>,
        Leon Romanovsky <leonro@...lanox.com>,
        RDMA mailing list <linux-rdma@...r.kernel.org>,
        Yishai Hadas <yishaih@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching
 event

On Tue, Jun 25, 2019 at 05:41:49PM +0300, Yishai Hadas wrote:
> > > > Why don't we return EIO as soon as is-destroyed happens? What is the
> > > > point of flushing out the accumulated events?
> > > 
> > > It follows the above uverb code/logic that returns existing events even in
> > > that case, also the async command events in this file follows that logic, I
> > > suggest to stay consistent.
> > 
> > Don't follow broken uverbs stuff...
> 
> May it be that there is some event that we still want to deliver post
> unbind/hot-unplug ? for example IB_EVENT_DEVICE_FATAL in uverbs and others
> from the driver code.

EIO is DEVICE_FATAL.

> Not sure that we want to change this logic.
> What do you think ?

I think this code should exit immediately with EIO if the device is
disassociated.

> > > > Maybe the event should be re-added on error? Tricky.
> > > 
> > > What will happen if another copy_to_user may then fail again (loop ?) ...
> > > not sure that we want to get into this tricky handling ...
> > > 
> > > As of above, It follows the logic from uverbs at that area.
> > > https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L267
> > 
> > again it is wrong...
> > 
> > There is no loop if you just stick the item back on the head of the
> > list and exit, which is probably the right thing to do..
> > 
> 
> What if copy_to_user() will fail again just later on ? we might end-up with
> loop of read(s) that always find an event as it was put back.

That is clearly an application bug and is not the concern of the
kernel..

> I suggest to leave this flow as it's now, at least for this series
> submission.
> 
> Agree ?

I don't think you can actually fix this, so maybe we have to leave
it. But add a comment explaining 

Jason