Re: [PATCH v7 10/12] sunrpc: add dst_attr attributes to the sysfs xprt directory

[Dan Aloni <dan@kernelim.com>]

On Mon, May 17, 2021 at 09:53:30AM -0400, Olga Kornievskaia wrote:
> On Sat, May 15, 2021 at 8:42 AM Dan Aloni <dan@kernelim.com> wrote:
> >
> > On Fri, May 14, 2021 at 10:13:21AM -0400, Olga Kornievskaia wrote:
> > > From: Olga Kornievskaia <kolga@netapp.com>
> > >
> > > Allow to query and set the destination's address of a transport.
> > > Setting of the destination address is allowed only for TCP or RDMA
> > > based connections.
> > >
> > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ..
> > > +     saddr = (struct sockaddr *)&xprt->addr;
> > > +     port = rpc_get_port(saddr);
> > > +
> > > +     dst_addr = kstrndup(buf, count - 1, GFP_KERNEL);
> > > +     if (!dst_addr)
> > > +             goto out_err;
> > > +     saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL);
> > > +     if (!saved_addr)
> > > +             goto out_err_free;
> > > +     saved_addr->addr =
> > > +             rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]);
> > > +     rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr);
> > > +     call_rcu(&saved_addr->rcu, free_xprt_addr);
> > > +     xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr,
> > > +                              sizeof(*saddr));
> >
> > Hi Olga,
> >
> > How does this behave if rpc_pton fails? Perhaps this conversion being
> > also a validation check on input given from user-space should be done
> > before the xprt is being modified?
> 
> It's assumed that an administrator is providing a valid (and correct)
> address value. Transport would continue to be disconnected until a
> proper value is supplied. We can't validate for instance that the
> supplied value is a "correct" value for the v4.1 server (which would
> require sending an EXCHANGE_ID and checking that it's the same server
> as before).

I'm not sure it would work that way - looks like `rpc_pton` zeroes
`saddr` before trying to fill it (or leaves it as it in other cases such
as the INET_ADDRSTRLEN bound check). I think `0.0.0.0` routes to
localhost by default? That might cause unexpected behavior from admin PoV.

More to consider is that `xprt->address_strings[RPC_DISPLAY_ADDR]`
should be consistent with the value of `saddr` in the error cases,
otherwise debugging xprt state may be give confusing output.

-- 
Dan Aloni