Re: [dm-devel] [PATCH 33/35] multipathd: uxlsnr: use poll loop for sending, too

[Martin Wilck <mwilck@suse.com>]

On Wed, 2021-09-15 at 23:22 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 10, 2021 at 01:41:18PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > send_packet() may busy-loop. By polling for POLLOUT, we can
> > avoid that, even if it's very unlikely in practice.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/uxlsnr.c | 39 ++++++++++++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 7 deletions(-)
> > 
> > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > index 1bf4126..c18b2c4 100644
> > --- a/multipathd/uxlsnr.c
> > +++ b/multipathd/uxlsnr.c
> > @@ -588,15 +588,37 @@ static void handle_client(struct client *c,
> > struct vectors *vecs, short revents)
> >                 if (get_strbuf_len(&c->reply) == 0)
> >                         default_reply(c, c->error);
> >  
> > -               const char *buf = get_strbuf_str(&c->reply);
> > +               if (c->cmd_len == 0) {
> > +                       size_t len = get_strbuf_len(&c->reply) + 1;
> >  
> > -               if (send_packet(c->fd, buf) != 0)
> > -                       dead_client(c);
> > -               else
> > -                       condlog(4, "cli[%d]: Reply [%zu bytes]", c-
> > >fd,
> > -                               get_strbuf_len(&c->reply) + 1);
> > -               reset_strbuf(&c->reply);
> > +                       if (send(c->fd, &len, sizeof(len),
> > MSG_NOSIGNAL)
> > +                           != sizeof(len)) {
> 
> This assumes that failing to send the size is always an error. What
> about if we get EINTR/EAGAIN? Also, it seems pretty likely that we will
> either send all of the size or none of it, but I'm not sure we can
> guarantee that.  send_packet() handled partitial writes of the length.
> Actually, mpath_recv_reply_len() which is still used by CLT_RECV still
> uses read_all(), instead of just polling again on partial reads.

That was intentional. I couldn't imagine that reading or writing 8
bytes would block (after all, we received POLLIN / POLLOUT for the
socket). Note the minimum socket buffer size that the kernel uses is
larger than 2048 bytes.

https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L2339

When we send the size value, the send buffer is empty by definition
(even for interactive sessions, if the client uses libmpathcmd, it will
have fetched the response before sending new requests). Likewise for
receiving, the size will be received in one piece if the client uses
libmpathcmd.

I take it that some malicious or badly designed client could access the
socket bypassing libmpathcmd, sending or receiving the size byte-by-
byte, or in other strange patterns. But I think we are within our
rights to error out in such cases; it may actually be the right thing
to do that, as we don't really want to service clients doing tricks
like that. I will do some tests.

> Also, the fd is not set to be non-blocking. and if we fall through to
> CLT_SEND, we haven't checked for a POLLOUT revent, so technically, I
> believe the write could block here.

Right, we have to do that. Thanks for pointing it out. This also means
that polling in read_all() and send_packet() was pointless in the first
place ...

Thanks!
Martin

PS: I was wondering why we haven't been using SOCK_DGRAM for the
multipath socket in the first place. It would have avoided all this
hassle...



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel