On Thu, Nov 15, 2018 at 04:03:59PM -0600, Steve Wise wrote:
> > >+ const struct rdma_link_ops *ops;
> > >+
> > >+ mutex_lock(&link_ops_mutex);
> > >+ list_for_each_entry(ops, &link_ops, list) {
> > >+ if (!strcmp(ops->type, type))
> > >+ goto out;
> > >+ }
> > >+ ops = NULL;
> > >+out:
> > >+ mutex_unlock(&link_ops_mutex);
> > >+ return ops;
> > >+}
> > >+
> > >+void rdma_link_register(struct rdma_link_ops *ops)
> > >+{
> > >+ if (link_ops_get(ops->type)) {
> > >+ WARN_ONCE("Duplicate rdma_link_ops! %s\n", ops->type);
> > >+ return;
> > >+ }
> > >+ mutex_lock(&link_ops_mutex);
> > >+ list_add(&ops->list, &link_ops);
> > >+ mutex_unlock(&link_ops_mutex);
> > >+}
> > >+EXPORT_SYMBOL(rdma_link_register);
> > >+
> > >+void rdma_link_unregister(struct rdma_link_ops *ops)
> > >+{
> > >+ mutex_lock(&link_ops_mutex);
> > >+ list_del(&ops->list);
> > >+ mutex_unlock(&link_ops_mutex);
> > >+}
> > >+EXPORT_SYMBOL(rdma_link_unregister);
> >
> > Will there be any issues with a link being unregistered while in use?
> >
>
> I guess what could happen is a NL message arrives and the ops ptr is
> fetched, but before it is used, the link is unregistered because the module
> is unloading. Then the function pointer is dereferenced and the memory has
> been freed and the module unloaded!
>
> Perhaps we just need to reference the module refcnt in link_ops_get() and
> add a link_ops_put() that deref's the module.
>
> Jason, is this a good approach?
No, rdma_link_unregister should not return while there are active
link_ops_get() callers.
I think you should have it be more like
mutex_lock(&link_ops_mutex);
list_for_each_entry(ops, &link_ops, list) ..
ops->newlink(...);
mutex_unlock(&link_ops_mutex);
So unregister serialization works as expected.
> > >+ goto err_out;
> >
> > Is err_out really necessary? Maybe just return?
> >
>
> Since there is no unwind code, I guess each of these failures could be just
> a return. But then if we end up adding unwind code, we'd want the goto's
> back in.
Generally we don't do the gotos if there is no current unwind
Even if there is unwind the goto's that only do 'return' are usually
kept as return
Jason
