Re: [PATCH 2/2] nfsd: expose /proc/net/sunrpc/nfsd in net namespaces

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-01-24 at 18:18 -0500, Josef Bacik wrote:
> On Wed, Jan 24, 2024 at 05:57:06PM -0500, Jeff Layton wrote:
> > On Wed, 2024-01-24 at 17:12 -0500, Josef Bacik wrote:
> > > On Wed, Jan 24, 2024 at 03:32:06PM -0500, Chuck Lever wrote:
> > > > On Wed, Jan 24, 2024 at 02:37:00PM -0500, Josef Bacik wrote:
> > > > > We are running nfsd servers inside of containers with their own network
> > > > > namespace, and we want to monitor these services using the stats found
> > > > > in /proc.  However these are not exposed in the proc inside of the
> > > > > container, so we have to bind mount the host /proc into our containers
> > > > > to get at this information.
> > > > > 
> > > > > Separate out the stat counters init and the proc registration, and move
> > > > > the proc registration into the pernet operations entry and exit points
> > > > > so that these stats can be exposed inside of network namespaces.
> > > > 
> > > > Maybe I missed something, but this looks like it exposes the global
> > > > stat counters to all net namespaces...? Is that an information leak?
> > > > As an administrator I might be surprised by that behavior.
> > > > 
> > > > Seems like this patch needs to make nfsdstats and nfsd_svcstats into
> > > > per-namespace objects as well.
> > > > 
> > > > 
> > > 
> > > I've got the patches written for this, but I've got a question.  There's a 
> > > 
> > > svc_seq_show(seq, &nfsd_svcstats);
> > > 
> > > in nfsd/stats.c.  This appears to be an empty struct, there's nothing that
> > > utilizes it, so this is always going to print 0 right?  There's a svc_info in
> > > the nfsd_net, and that stats block appears to get updated properly.  Should I
> > > print this out here?  I don't see anywhere we get the rpc stats out of nfsd, am
> > > I missing something?  I don't want to rip out stuff that I don't quite
> > > understand.  Thanks,
> > > 
> > > 
> > 
> > nfsd_svcstats ends up being the sv_stats for the nfsd service. The RPC
> > code has some counters in there for counting different sorts of net and
> > rpc events (see svc_process_common, and some of the recv and accept
> > handlers).  I think nfsstat(8) may fetch that info via the above
> > seqfile, so it's definitely not unused (and it should be printing more
> > than just a '0').
> 
> Ahhh, I missed this bit
> 
> struct svc_program              nfsd_program = {
> #if defined(CONFIG_NFSD_V2_ACL) || defined(CONFIG_NFSD_V3_ACL)
>         .pg_next                = &nfsd_acl_program,
> #endif
>         .pg_prog                = NFS_PROGRAM,          /* program number */
>         .pg_nvers               = NFSD_NRVERS,          /* nr of entries in
> nfsd_version */
>         .pg_vers                = nfsd_version,         /* version table */
>         .pg_name                = "nfsd",               /* program name */
>         .pg_class               = "nfsd",               /* authentication class
> */
>         .pg_stats               = &nfsd_svcstats,       /* version table */
>         .pg_authenticate        = &svc_set_client,      /* export authentication
> */
>         .pg_init_request        = nfsd_init_request,
>         .pg_rpcbind_set         = nfsd_rpcbind_set,
> };
> 
> and so nfsd_svcstats definitely is getting used.
> 
> > 
> > svc_info is a completely different thing: it's a container for the
> > svc_serv...so I'm not sure I understand your question?
> 
> I was just confused, and still am a little bit.
> 
> The counters are easy, I put those into the nfsd_net struct and make everything
> mess with those counters and report those from proc.
> 
> However the nfsd_svcstats are in this svc_program thing, which appears to need
> to be global?  Or do I need to make it per net as well?  Or do I need to do
> something completely different to track the rpc stats per network namespace?

Making the svc_program per-net is unnecessary for this (and probably not
desirable). That structure sort of describes the nfsd rpc "program" and
that is pretty much the same between containers.

I think making having a different sv_stats per-namespace makes sense.
It'll be a departure from the way it works today though. Looking at
nfsstat in the init_ns will no longer show global counters. I don't
think it's a bad change, but it will be a change that we'll need to take
into account (and maybe document).

This is all really old, crusty code, and some of it like the sv_stats
code originates from the 90s. Right now, sv_stats is only assigned in
svc_create and it comes from the svc_program. You'll need to do
something different there.

Now that I look too, it looks like we're just doing bare increments to
the counters without any locking, which seems a bit racy. I wonder
whether we ought to be doing something percpu there instead?
-- 
Jeff Layton <jlayton@kernel.org>