On Tue, Feb 05, 2019 at 08:17:43AM +0200, Leon Romanovsky wrote:
> > .. so if we are now relying on the work queue flush to make this
> > correct, I think the mlx5_ib_prefetch_mr_work could be written a bit
> > more clearly:
> >
> > if (WARN_ON(!ib_device_try_get(&w->dev->ib_dev)))
> > goto out;
> >
> > mlx5_ib_prefetch_sg_list(w->dev, w->pf_flags, w->sg_list,
> > w->num_sge);
> > ib_device_put(&w->dev->ib_dev);
> > out:
> >
> > As it is no longer acceptable for work to be executed on an
> > unregistered ib device, this means that the MR's were not flushed
> > before all the clients exited, which should not be possible.
>
> Why should we do ib_device get/put? We are already operating on such
> device and it should be "with us" all the time we are executing
> mlx5_ib_prefetch_sg_list().
Indeed, that should all be deleted too and replaced with a comment.
> > > INIT_WORK(&work->work, mlx5_ib_prefetch_mr_work);
> > > - schedule_work(&work->work);
> > > - return 0;
> > > +
> > > + srcu_key = srcu_read_lock(&dev->mr_srcu);
> > > + for (i = 0; i < num_sge; ++i) {
> >
> > 'num_sge' is read directly from userspace and it is a u32, but 'i'
> > here is an 'int', so userspace can trigger and infinite loop in the
> > kernel. I noticed this type mistake in other similar places here as
> > well.
> >
> > Using 'int' for a for loop variable is almost always the wrong thing
> > to do in some subtle way like this..
>
> It is true for 32bit systems where "int" is 32 bits length. Those
> systems are under radar for us anyway.
'int' is always 32 bit
> > > + struct mlx5_core_mkey *mmkey;
> > > + struct mlx5_ib_mr *mr;
> > > +
> > > + mmkey = __mlx5_mr_lookup(dev->mdev,
> > > + mlx5_base_mkey(sg_list[i].lkey));
> > > + if (!mmkey || mmkey->key != sg_list[i].lkey) {
> > > + valid_req = false;
> > > + break;
> > > + }
> > > +
> > > + if (mmkey->type != MLX5_MKEY_MR) {
> > > + valid_req = false;
> > > + break;
> > > + }
> > > +
> > > + mr = container_of(mmkey, struct mlx5_ib_mr, mmkey);
> > > +
> > > + if (mr->ibmr.pd != pd) {
> > > + valid_req = false;
> > > + break;
> > > + }
> > > +
> > > + if (!mr->live) {
> > > + valid_req = false;
> > > + break;
> > > + }
> > > +
> > > + atomic_inc(&mr->num_pending_prefetch);
> > > + }
> >
> > This would be nicer in a function 'incr_num_pending_prefetch' or some
> > such
>
> Maybe, I'm not fun of one-time called functions.
It is useful if it fixes error unwind control flow messes..
Jason
