On Sun, Jan 13, 2019 at 10:26:03AM -0700, Parav Pandit wrote: > > > > From: Moni Shoua <monis@xxxxxxxxxxxx> > > Sent: Sunday, January 13, 2019 11:19 AM > > To: Jason Gunthorpe <jgg@xxxxxxxxxxxx> > > Cc: Parav Pandit <parav@xxxxxxxxxxxx>; Leon Romanovsky > > <leon@xxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH for-rc 2/2] IB/mlx5: Fix how advise_mr() launches async > > work > > > > On Sat, Jan 12, 2019 at 8:42 PM Jason Gunthorpe <jgg@xxxxxxxxxxxx> > > wrote: > > > > > > On Sat, Jan 12, 2019 at 11:06:51AM -0700, Parav Pandit wrote: > > > > > > > > > > > > > From: Jason Gunthorpe > > > > > Sent: Saturday, January 12, 2019 11:39 AM > > > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx; > > > > > Moni Shoua <monis@xxxxxxxxxxxx> > > > > > Subject: Re: [PATCH for-rc 2/2] IB/mlx5: Fix how advise_mr() > > > > > launches async work > > > > > > > > > > On Sat, Jan 12, 2019 at 10:32:54AM -0700, Parav Pandit wrote: > > > > > > > > > > > > > > > > > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma- > > > > > > > owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe > > > > > > > Sent: Saturday, January 12, 2019 11:06 AM > > > > > > > To: Leon Romanovsky <leon@xxxxxxxxxx> > > > > > > > Cc: linux-rdma@xxxxxxxxxxxxxxx; Moni Shoua > > > > > > > <monis@xxxxxxxxxxxx> > > > > > > > Subject: Re: [PATCH for-rc 2/2] IB/mlx5: Fix how advise_mr() > > > > > > > launches async work > > > > > > > > > > > > > > On Sat, Jan 12, 2019 at 05:06:02PM +0200, Leon Romanovsky wrote: > > > > > > > > On Fri, Jan 11, 2019 at 07:31:24PM -0700, Jason Gunthorpe wrote: > > > > > > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx> > > > > > > > > > > > > > > > > > > Work must hold a kref on the ib_device otherwise the dev > > > > > > > > > pointer can become free before the work runs. > > > > > > > > > > > > > > > > How can it occur exactly? > > > > > > > > > > > > > > > > "dev" will be released after mlx5_ib will exist, mlx5_ib > > > > > > > > exist will drain advise_mr queue before it. Such drain will > > > > > > > > ensure that no works are running. > > > > > > > > > > > > > > Okay, that is fine for the get_device, it can be removed. > > > > > > > > > > > > > > Are we OK with the reg_state hunk? > > > > > > > > > > > > > > Jason > > > > > > > > > > > > Unrelated to your question, I noticed that advise_mr does > > > > > > uverbs_attr_ptr_get_array_size() for the SGEs. I think this is > > > > > > incorrect. It should have been mr handles whose ownership can > > > > > > be check against the PID or ucontext that registered those MRs. > > > > > > > > > > The SGE's have lkeys in them, and the lkeys are local to the PD > > > > > passed in as argument. > > > > > > > > > > So the caller cannot touch resources outside the PD, hopefully the > > > > > driver implementation checks this.. > > > > > > > > > pagefault_single_data_segment () invokes __mlx5_mr_lookup() to > > > > lookup a MR per device and not per PD. mlx5_ib_advise_mr_prefetch() > > > > doesn't touch pd. work item doesn't have PD in it. > > > > > > Ugh, Moni? > > > > > > I don't think we should alow processes to page fault outside their PD, > > > that sounds like an avenue for a side-channel security problem. > > > > > > > I can pass pd from prefetch_mr down to pagefault_single_data_segment() > > and force that that lookup will be also for pd. > > However there is another flow that goes through > > pagefault_single_data_segment() that starts from hardware page-fault. > > What will be the pd in this case? Perhaps the pd of the source/destination > > QP of the packet that generated the pagefault? But I can do this only if the > > type of page-fault is WQE. In this case the hardware provides the QPN. For > > page-fault of type RDMA the hardware doesn't provide the QPN, only the > > lkey, address and size (dev is the one that fired the event). > > So if there is a potential security risk in the advise_mr flow doesn't it exist in > > the page-fault flow? A malicious packet can do the same thing a malicious > > process does. > > What do you think? > > When hw triggered the page fault it has passed the checks of PD for > a QP and MR from the QP and MR context including honoring > read/write/atomic access rights. Once those checks pass, it should > ask for page fault. If MR gets destroyed in this process before > page fault, host driver will be terminating the MR and all pending > transactions. I agree, the HW needs to validate all on-the-wire rkeys against the Rx'ing QP's PD before passing them into the driver. So if the HW is right then the HW driver flow should ignore the PD as it is already checked. Jason
