DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BB39520657
MIME-Version: 1.0
References: <20181019152905.32418-1-olga.kornievskaia@gmail.com>
 <20181019152905.32418-7-olga.kornievskaia@gmail.com> <20181101203727.GC12930@fieldses.org>
In-Reply-To: <20181101203727.GC12930@fieldses.org>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Thu, 1 Nov 2018 16:55:24 -0400
Message-ID: <CAN-5tyEbjhKLH5yp8gTvJn9Ua0Vrq5Eep6kRuUgX7S2MDjO-Fw@mail.gmail.com>
Subject: Re: [PATCH v1 06/13] NFSD fill-in netloc4 structure
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 1, 2018 at 4:37 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Oct 19, 2018 at 11:28:58AM -0400, Olga Kornievskaia wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > nfs.4 defines nfs42_netaddr structure that represents netloc4.
> >
> > Populate needed fields from the sockaddr structure.
> >
> > This will be used by flexfiles and 4.2 inter copy
> >
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfsd/nfsd.h | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> > index 0668999..030fccb 100644
> > --- a/fs/nfsd/nfsd.h
> > +++ b/fs/nfsd/nfsd.h
> > @@ -18,7 +18,7 @@
> >  #include <linux/nfs4.h>
> >  #include <linux/sunrpc/svc.h>
> >  #include <linux/sunrpc/msg_prot.h>
> > -
> > +#include <linux/sunrpc/addr.h>
> >  #include <uapi/linux/nfsd/debug.h>
> >
> >  #include "stats.h"
> > @@ -366,6 +366,35 @@ static inline bool nfsd4_spo_must_allow(struct svc_rqst *rqstp)
> >
> >  extern const u32 nfsd_suppattrs[3][3];
> >
> > +static inline u32 nfsd4_set_netaddr(struct sockaddr *addr, struct nfs42_netaddr *netaddr)
> > +{
> > +     struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> > +     struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)addr;
> > +     unsigned int port;
> > +     size_t ret;
> > +
> > +     switch (addr->sa_family) {
> > +     case AF_INET:
> > +             port = ntohs(sin->sin_port);
> > +             sprintf(netaddr->netid, "tcp");
> > +             netaddr->netid_len = 3;
> > +             break;
> > +     case AF_INET6:
> > +             port = ntohs(sin6->sin6_port);
> > +             sprintf(netaddr->netid, "tcp6");
> > +             netaddr->netid_len = 4;
> > +             break;
> > +     default:
> > +             return nfserrno(-EINVAL);
>
> Let's just write that nfserr_inval;

Ok will do.

>
> > +     }
> > +     ret = rpc_ntop(addr, netaddr->addr, sizeof(netaddr->addr));
> > +     netaddr->addr_len = ret + snprintf(netaddr->addr + ret,
> > +                                  RPCBIND_MAXUADDRLEN + 1,
> > +                                  ".%u.%u", port >> 8, port & 0xff);
>
> Isn't the remaining space RPCBIND_MAXUADDRLEN + 1 - ret ?

You are right.

> But really I think we're depending on RPCBIND_MAXUADDRLEN being large
> enough that there will never be overlow, in which case we may as well
> just use sprintf.
>
> And maybe add a WARN_ON() in case of overflow, if we want to be
> paranoid about it.

Are you proposing to first get the output from snprintf() and see if
there was an overflow because it returned more than the supplied size
and then print WARN_ON()? How do I detect an overflow with just a
sprintf()?

>
> --b.
>
> > +
> > +     return 0;
> > +}
> > +
> >  static inline bool bmval_is_subset(const u32 *bm1, const u32 *bm2)
> >  {
> >       return !((bm1[0] & ~bm2[0]) ||
> > --
> > 1.8.3.1