From: Jason Gunthorpe <jgg@nvidia.com>
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: <linux-rdma@vger.kernel.org>, Bob Pearson <rpearson@hpe.com>
Subject: Re: [PATCH RFC] rdma_rxe: Stop passing AV from user space
Date: Mon, 19 Oct 2020 20:00:17 -0300 [thread overview]
Message-ID: <20201019230017.GB6219@nvidia.com> (raw)
In-Reply-To: <c8e69967-12aa-6f8e-18c5-96fbd9f1dc2b@gmail.com>
On Mon, Oct 19, 2020 at 02:06:30PM -0500, Bob Pearson wrote:
> On 10/19/20 1:53 PM, Jason Gunthorpe wrote:
> > On Fri, Oct 16, 2020 at 12:01:48PM -0500, Bob Pearson wrote:
> >>
> >> +static struct ib_ah *get_ah_from_handle(struct rxe_qp *qp, u32 handle)
> >> +{
> >> + struct ib_uverbs_file *ufile;
> >> + struct uverbs_api *uapi;
> >> + const struct uverbs_api_object *type;
> >> + struct ib_uobject *uobj;
> >> +
> >> + ufile = qp->ibqp.uobject->uevent.uobject.ufile;
> >> + uapi = ufile->device->uapi;
> >> + type = uapi_get_object(uapi, UVERBS_OBJECT_AH);
> >> + if (IS_ERR(type))
> >> + return NULL;
> >> + uobj = rdma_lookup_get_uobject(type, ufile, (s64)handle,
> >> + UVERBS_LOOKUP_READ, NULL);
> >> + if (IS_ERR(uobj)) {
> >> + pr_warn("unable to lookup ah handle\n");
> >> + return NULL;
> >> + }
> >> +
> >> + rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_READ);
> >
> > It can't be put and then return the data pointer, it is a use after free:
> >
> >> + return uobj->object;
> >
> >> @@ -562,11 +563,6 @@ static int init_send_wqe(struct rxe_qp *qp, const struct ib_send_wr *ibwr,
> >>
> >> init_send_wr(qp, &wqe->wr, ibwr);
> >>
> >> - if (qp_type(qp) == IB_QPT_UD ||
> >> - qp_type(qp) == IB_QPT_SMI ||
> >> - qp_type(qp) == IB_QPT_GSI)
> >> - memcpy(&wqe->av, &to_rah(ud_wr(ibwr)->ah)->av, sizeof(wqe->av));
> >
> > It needs some kind of negotiated compat, can't just break userspace
> > like this
> >
> > Jason
> >
>
> 1st point. I get it. uobj->object contains the address of one of the ib_xxx verbs objects.
> Normally the driver never looks at this level but presumably has a kref on that object so it makes
> sense to look it up. Perhaps better would be:
>
> void *object;
>
> ...
>
> uobj = rdma_lookup_get_uobject(...);
>
> object = uobj->object;
>
> rdma_lookup_put_uobject(...);
>
> return (struct ib_ah *)object;
>
> Here the caller has created the ib_ah but has not yet destroyed it so it must hold a kref on it.
Drivers are not supposed to keep using object after it has been
destroyed, so some kind of interlock is needed to prevent/defer
destruction here.
The uobject layer does not provide something usable to drivers
> 2nd point. I also get. This suggestion imagines that there will come
> a day when we can change the user API. May be a rare day but must
> happen occasionally. The current design is just plain wrong and
> needs to get fixed eventually.
You can have some cap negotiation to switch the mode AH mode in the
WQEs - 'Use WQE format 2' for instance. Most of the HW drivers have
multiple WQE formats the userspace selects.
Jason
prev parent reply other threads:[~2020-10-19 23:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-16 17:01 [PATCH RFC] rdma_rxe: Stop passing AV from user space Bob Pearson
2020-10-19 18:53 ` Jason Gunthorpe
2020-10-19 19:06 ` Bob Pearson
2020-10-19 23:00 ` Jason Gunthorpe [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201019230017.GB6219@nvidia.com \
--to=jgg@nvidia.com \
--cc=linux-rdma@vger.kernel.org \
--cc=rpearson@hpe.com \
--cc=rpearsonhpe@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).