Re: [PATCH] RDMA/core: Add wait/retry version of ibnl_unicast

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6109 bytes --]

On Wed, Jun 28, 2017 at 03:30:03PM -0500, Chien Tin Tung wrote:
> On Wed, Jun 28, 2017 at 06:36:39PM +0300, Leon Romanovsky wrote:
> > On Wed, Jun 28, 2017 at 09:12:11AM -0500, Chien Tin Tung wrote:
> > > On Wed, Jun 28, 2017 at 09:02:45AM -0500, Mustafa Ismail wrote:
> > > > Add a wait/retry version of ibnl_unicast, ibnl_unicast_wait,
> > > > and modify ibnl_unicast to not wait/retry.  This eliminates
> > > > the undesirable wait for future users of ibnl_unicast.
> > > >
> > > > Change Portmapper calls originating from kernel to user-space
> > > > to use ibnl_unicast_wait and take advantage of the wait/retry
> > > > logic in netlink_unicast.
> > > >
> > > > Signed-off-by: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > Signed-off-by: Chien Tin Tung <chien.tin.tung-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > ---
> > > >  drivers/infiniband/core/iwpm_msg.c |  6 +++---
> > > >  drivers/infiniband/core/netlink.c  | 12 +++++++++++-
> > > >  include/rdma/rdma_netlink.h        | 10 ++++++++++
> > > >  3 files changed, 24 insertions(+), 4 deletions(-)
> > >
> > > Please apply this patch instead of Leon's patch to revert
> > > "IB/core: Add flow control to the portmapper netlink calls".
> > >
> > > Leon, we can work out names and parameters if this works for you.
> >
> > Chien,
> >
> > The names are less my worries with this patch. First of all, it misleads
> > by using wait/retry naming, because it blocks and not waits.
>
> Nope.  It does a single shot retry and waits in a waitqueue.
> Go look at netlink_unicast and in turn netlink_attachskb.  If you still
> disagree, please flag specific code where it blocks.

I agree, it wouldn't block in your scenario. However will it work in more hostile
environments?

For example, malicious user can open RDMA netlink socket directly (socket(...)),
set sndtimeo to be MAX_SCHEDULE_TIMEOUT - 1 (LONG_MAX - 1) and send custom
netlink messages right to your new _wait function. If I understand correctly
from the code, it will add them to waitqueue and won't release skb till
the end of processing.

Will it cause to mark whole netlink socket as NETLINK_S_CONGESTED?
Will other users will be able to progress with their messages or they
will need to wait till those _wait calls finish?

>
> Here are the two functions for your convenience.
>
>
> int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
>                     u32 portid, int nonblock)
> {
>         struct sock *sk;
>         int err;
>         long timeo;
>
>         skb = netlink_trim(skb, gfp_any());
>
>         timeo = sock_sndtimeo(ssk, nonblock);
> retry:
>         sk = netlink_getsockbyportid(ssk, portid);
>         if (IS_ERR(sk)) {
>                 kfree_skb(skb);
>                 return PTR_ERR(sk);
>         }
>         if (netlink_is_kernel(sk))
>                 return netlink_unicast_kernel(sk, skb, ssk);
>
>         if (sk_filter(sk, skb)) {
>                 err = skb->len;
>                 kfree_skb(skb);
>                 sock_put(sk);
>                 return err;
>         }
>
>         err = netlink_attachskb(sk, skb, &timeo, ssk);
>         if (err == 1)
>                 goto retry;
>         if (err)
>                 return err;
>
>         return netlink_sendskb(sk, skb);
> }
>
> /*
>  * Attach a skb to a netlink socket.
>  * The caller must hold a reference to the destination socket. On error, the
>  * reference is dropped. The skb is not send to the destination, just all
>  * all error checks are performed and memory in the queue is reserved.
>  * Return values:
>  * < 0: error. skb freed, reference to sock dropped.
>  * 0: continue
>  * 1: repeat lookup - reference dropped while waiting for socket memory.
>  */
> int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
>                       long *timeo, struct sock *ssk)
> {
>         struct netlink_sock *nlk;
>
>         nlk = nlk_sk(sk);
>
>         if ((atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf ||
>              test_bit(NETLINK_S_CONGESTED, &nlk->state))) {
>                 DECLARE_WAITQUEUE(wait, current);
>                 if (!*timeo) {
>                         if (!ssk || netlink_is_kernel(ssk))
>                                 netlink_overrun(sk);
>                         sock_put(sk);
>                         kfree_skb(skb);
>                         return -EAGAIN;
>                 }
>
>                 __set_current_state(TASK_INTERRUPTIBLE);
>                 add_wait_queue(&nlk->wait, &wait);
>
>                 if ((atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf ||
>                      test_bit(NETLINK_S_CONGESTED, &nlk->state)) &&
>                     !sock_flag(sk, SOCK_DEAD))
>                         *timeo = schedule_timeout(*timeo);
>
>                 __set_current_state(TASK_RUNNING);
>                 remove_wait_queue(&nlk->wait, &wait);
>                 sock_put(sk);
>
>                 if (signal_pending(current)) {
>                         kfree_skb(skb);
>                         return sock_intr_errno(*timeo);
>                 }
>                 return 1;
>         }
>         netlink_skb_set_owner_r(skb, sk);
>         return 0;
> }
>
>
> BTW, _nobody_ is resetting the socket attribute from O_NONBLOCK.
>
> It is very difficult to understand your argument of "blocking" when you are not
> sharing the specifics.  Please put your finger on it so everyone can understand
> your point.

I hope that I succeeded to answer on your questions.

>
> > The second, I disagree with solution in kernel for user space application which can't
> > handle the netlink errors.
>
> There is no guarantee delivery nor blocking on send.  Like I mentioned above,
> it is a 1 shot retry with a set wait time.  The code obviousely handles error
> condition as it can happen.

So, can you please refresh our memory and explain again what exactly
this patch is fixing if user-space handles errors correctly?

>
>
> Chien

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]