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;
Date:   Mon, 17 May 2021 20:16:13 +0300
From:   Dan Aloni <dan@kernelim.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v7 10/12] sunrpc: add dst_attr attributes to the sysfs
 xprt directory
Message-ID: <20210517171613.toxgdygnzw76ibgd@gmail.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAN-5tyGGnrGPMq4kakCZhNdJj9i-Arc-FXn=uxWRW9nuAVpE7A@mail.gmail.com>
Precedence: bulk

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