Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210514141323.67922-1-olga.kornievskaia@gmail.com>
 <20210514141323.67922-11-olga.kornievskaia@gmail.com> <20210515124232.y7xmofawebp7l5w6@gmail.com>
 <CAN-5tyGGnrGPMq4kakCZhNdJj9i-Arc-FXn=uxWRW9nuAVpE7A@mail.gmail.com> <20210517171613.toxgdygnzw76ibgd@gmail.com>
In-Reply-To: <20210517171613.toxgdygnzw76ibgd@gmail.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Mon, 17 May 2021 14:14:13 -0400
Message-ID: <CAN-5tyGV+Ct6ybKAJG98wLpJE=V70D=oXLQq9CdNh92iMWcisA@mail.gmail.com>
Subject: Re: [PATCH v7 10/12] sunrpc: add dst_attr attributes to the sysfs
 xprt directory
To:     Dan Aloni <dan@kernelim.com>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, May 17, 2021 at 1:16 PM Dan Aloni <dan@kernelim.com> wrote:
>
> 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.

the reason for rpc_pton() to fail if an incorrect address is supplied.
I again said that given that this is a privileged operation, we rely
on the admin to provide a correct address. But are you arguing for
checking return or rpt_pton() and then reverting address_string
assignment back to the saved value and returning? Ok, I can do that.
But what can't be done is that once xprt_force_disconnect() is called
(1) we can't know if re-connect to the newly provided value is
successful and (2) thus we can't roll back the address values to the
old ones. Seems the added complexity is not gaining us much in return.
As I said I think the userspace tool that would sit on top of this
would do all the checks first before using the sysfs.

>
> --
> Dan Aloni