On Mon, May 17, 2021 at 09:53:30AM -0400, Olga Kornievskaia wrote:

On Sat, May 15, 2021 at 8:42 AM Dan Aloni [off-list ref] wrote:

quoted

On Fri, May 14, 2021 at 10:13:21AM -0400, Olga Kornievskaia wrote:

quoted

From: Olga Kornievskaia <redacted>

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 <redacted>

..

quoted

+     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