[Openib-windows] RE: User verbs
Fabian Tillier
ftillier at silverstorm.com
Thu Mar 2 18:04:51 PST 2006
Hi Leonid,
On 3/2/06, Leonid Keller <leonid at mellanox.co.il> wrote:
> Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c
> ===================================================================
> --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c (revision 401)
> +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c (revision 1031)
> @@ -582,6 +582,7 @@
> {
> /* Copy the dev info. */
> p_um_ca->dev_info = *hca_ul_info;
> + p_um_ca->hob_p = hob_p;
> *ph_um_ca = (ib_ca_handle_t)p_um_ca;
> (*(void** __ptr64)p_umv_buf->p_inout_buf) =
> p_um_ca->p_mapped_addr;
> p_umv_buf->status = IB_SUCCESS;
> @@ -644,7 +645,7 @@
> OUT ib_pd_handle_t
> *ph_pd,
> IN OUT ci_umv_buf_t
> *p_umv_buf )
> {
> - mlnx_hob_t *hob_p = (mlnx_hob_t
> *)h_ca;
> + mlnx_hob_t *hob_p;
> mlnx_hobul_t *hobul_p;
> HH_hca_dev_t *hca_ul_info;
> HHUL_pd_hndl_t hhul_pd_hndl = 0;
> @@ -655,6 +656,11 @@
>
> CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);
>
> + if( p_umv_buf && p_umv_buf->command )
> + hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
> + else
> + hob_p = (mlnx_hob_t *)h_ca;
This won't work properly - when the CA is opened by a user-mode
process without the kernel bypass provider (either it is missing,
corrupted, or whatever other failure), the p_umv_buf->command is zero
to mlnx_um_ca, in which case it returns NULL for the mlnx_um_ca_t
object. This will cause a NULL dereference in the code above.
There are a few ways of fixing this. First, we can keep checking the
command, and in mlnx_um_open return the mlnx_hob_t if command is zero.
This way h_um_ca is the same as h_ca.
Another way is to always allocate the mlnx_um_ca_t object, even if
p_umv_buf->command is zero, and remove the check for the command when
resolving the HCA object.
I'm leaning towards the first, since it's simplest. I'll implement
that and send out a patch for approval.
- Fab
> hobul_p = mlnx_hobs_get_hobul(hob_p);
> if (NULL == hobul_p) {
> status = IB_INVALID_CA_HANDLE;
> @@ -1889,7 +1895,7 @@
> {
> ib_api_status_t status;
>
> - mlnx_hob_t *hob_p = (mlnx_hob_t
> *)h_ca;
> + mlnx_hob_t *hob_p;
> u_int32_t cq_idx;
> u_int32_t cq_num;
> u_int32_t cq_size = 0;
> @@ -1901,6 +1907,11 @@
>
> CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);
>
> + if( p_umv_buf && p_umv_buf->command )
> + hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
> + else
> + hob_p = (mlnx_hob_t *)h_ca;
> +
> hobul_p = mlnx_hobs_get_hobul(hob_p);
> if (NULL == hobul_p) {
> status = IB_INVALID_CA_HANDLE;
>
>
>
> > -----Original Message-----
> > From: Fab Tillier [mailto:ftillier at silverstorm.com]
> > Sent: Wednesday, January 11, 2006 7:19 PM
> > To: Leonid Keller
> > Subject: RE: User verbs
> >
> > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > Sent: Wednesday, January 11, 2006 8:45 AM
> > >
> > > > From: Fab Tillier [mailto:ftillier at silverstorm.com]
> > > > Sent: Tuesday, January 10, 2006 8:05 PM
> > > >
> > > > Hi Leo,
> > > >
> > > > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > > > Sent: Tuesday, January 10, 2006 8:45 AM
> > > > >
> > > > > Hi Fab !
> > > > >
> > > > > Working on porting of the user part of the MTHCA driver i came
> > > > > accross the following question:
> > > > > Why do you use h_ca (kernel HCA handle) and not h_um_ca
> > (context,
> > > > > returned by mlnx_um_open) in calls to call mlnx_allocate_pd and
> > > > > mlnx_create_cq ?
> > > >
> > > > The problem comes from the fact that mlnx_um_open isn't
> > called for
> > > > all clients (not kernel ones), so passing h_um_ca to the
> > verbs calls
> > > > would break kernel clients.
> > >
> > > You didn't understand my suggestion, I guess.
> >
> > I had totally missed it!
> >
> > > It's quite natural, that user clients have their own _um_open call.
> > > It's just an old good work paradigm: h = open(); verb(h);
> > ...; close(h).
> > > Pay attention, that h_ca and h_um_ca have the same type
> > ib_ca_handle_t.
> > > What I'm suggesting is to call alloc_pd and create_cq with h_ca for
> > > kernel clients and with h_um_ca - for user ones. The verbs
> > will choose
> > > the right casting according to the value of p_umv_buf.
> >
> > But this creates an inconsistency with other verbs, which may
> > also benefit from having the h_um_ca. ci_call is just an
> > example of a call that would benefit, as it would allow the
> > HCA driver to store any CR space mapping information in the
> > h_um_ca structure, making CR space mapping into UM safe.
> >
> > So, I'm OK with changing all the verbs to check p_umv_buf and
> > then doing the proper cast. However, in light of the
> > direction I expect the verbs to go in (where the user
> > multiplexing on open is done in the HCA driver and not IBAL),
> > having all open calls result in some sort of user open call
> > and then using that handle exclusively in all verb calls
> > would align better.
> >
> > That is, rename um_open_ca to user_open_ca, and call it for
> > all clients. For kernel clients, p_umv_buf would be NULL and
> > the HCA driver would know not to try to map resources into
> > user-mode. Then, every call changes to take the h_um_ca.
> >
> > Anyhow, whether we change every call to check p_umv_buf, or
> > change um_open_ca and change the type of the structure for
> > every other verb is pretty much equivalent. Plus it lets us
> > merge the CR space mapping stuff in time for the OpenIB 1.0 release.
> >
> > What do you think?
> >
> > - Fab
> >
> >
> _______________________________________________
> openib-windows mailing list
> openib-windows at openib.org
> http://openib.org/mailman/listinfo/openib-windows
>
More information about the ofw
mailing list