Re: [PATCH v2] RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init

[Leon Romanovsky <leon@kernel.org>]

On Wed, Aug 05, 2020 at 01:18:05PM +0200, Danil Kipnis wrote:
> On Wed, Aug 5, 2020 at 11:16 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Aug 05, 2020 at 11:09:00AM +0200, Danil Kipnis wrote:
> > > Hi Leon,
> > >
> > > On Wed, Aug 5, 2020 at 7:57 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Tue, Aug 04, 2020 at 07:07:58PM +0530, Md Haris Iqbal wrote:
> > > > > The rnbd_server module's communication manager (cm) initialization depends
> > > > > on the registration of the "network namespace subsystem" of the RDMA CM
> > > > > agent module. As such, when the kernel is configured to load the
> > > > > rnbd_server and the RDMA cma module during initialization; and if the
> > > > > rnbd_server module is initialized before RDMA cma module, a null ptr
> > > > > dereference occurs during the RDMA bind operation.
> > > > >
> > > > > Call trace below,
> > > > >
> > > > > [    1.904782] Call Trace:
> > > > > [    1.904782]  ? xas_load+0xd/0x80
> > > > > [    1.904782]  xa_load+0x47/0x80
> > > > > [    1.904782]  cma_ps_find+0x44/0x70
> > > > > [    1.904782]  rdma_bind_addr+0x782/0x8b0
> > > > > [    1.904782]  ? get_random_bytes+0x35/0x40
> > > > > [    1.904782]  rtrs_srv_cm_init+0x50/0x80
> > > > > [    1.904782]  rtrs_srv_open+0x102/0x180
> > > > > [    1.904782]  ? rnbd_client_init+0x6e/0x6e
> > > > > [    1.904782]  rnbd_srv_init_module+0x34/0x84
> > > > > [    1.904782]  ? rnbd_client_init+0x6e/0x6e
> > > > > [    1.904782]  do_one_initcall+0x4a/0x200
> > > > > [    1.904782]  kernel_init_freeable+0x1f1/0x26e
> > > > > [    1.904782]  ? rest_init+0xb0/0xb0
> > > > > [    1.904782]  kernel_init+0xe/0x100
> > > > > [    1.904782]  ret_from_fork+0x22/0x30
> > > > > [    1.904782] Modules linked in:
> > > > > [    1.904782] CR2: 0000000000000015
> > > > > [    1.904782] ---[ end trace c42df88d6c7b0a48 ]---
> > > > >
> > > > > All this happens cause the cm init is in the call chain of the module init,
> > > > > which is not a preferred practice.
> > > > >
> > > > > So remove the call to rdma_create_id() from the module init call chain.
> > > > > Instead register rtrs-srv as an ib client, which makes sure that the
> > > > > rdma_create_id() is called only when an ib device is added.
> > > > >
> > > > > Fixes: 9cb837480424 ("RDMA/rtrs: server: main functionality")
> > > > > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > > > > ---
> > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 77 +++++++++++++++++++++++++-
> > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.h |  7 +++
> > > > >  2 files changed, 81 insertions(+), 3 deletions(-)
> > > >
> > > > Please don't send vX patches as reply-to in "git send-email" command.
> > >
> > > I thought vX + in-reply-to makes it clear that a new version of a
> > > patch is proposed in response to a mail reporting a problem in the
> > > first version. Why is that a bad idea?
> >
> > It looks very messy in e-mail client. It is hard to follow and requires
> > multiple iterations to understand if the reply is on previous variant or
> > on new one.
> >
> > See attached print screen or see it in lore, where thread view is used.
> > https://lore.kernel.org/linux-rdma/20200623172321.GC6578@ziepe.ca/T/#t
>
> I see, thanks. Just a related question: In this particular case the
> commit message changed in the second version of the patch. Should one
> still use v2 in that case, or should the second version be submitted
> without the vX tag at all?

It depends on the change itself. If title wasn't changed, the vX would
help to distinguish between versions. Once title changes and the commit
message too, the vX is not really needed (IMHO).

Thanks

>
> >
> > Thanks