Re: [Libtirpc-devel] Fwd: Re: proposed patch to rpcbind to providefiner-grained security controls than offered by the -i option

[Jeff Layton <jlayton at redhat dot com>]

On Sat, 18 Dec 2010 18:01:42 -0500
Chuck Lever <chucklever@gmail.com> wrote:

> Hi Andrew-
> 
> On Sat, Dec 18, 2010 at 5:44 PM, Andrew J. Schorr
> <ajschorr@alumni.princeton.edu> wrote:
> > On Sat, Dec 18, 2010 at 05:33:54PM -0500, Chuck Lever wrote:
> >> If there is a common solution to this issue among RPC implementations,
> >> then we should adopt that.
> >>
> >> The discussion we had 9 months ago was among a former Sun NFS
> >> engineer, a CIFS developer, and myself (10 years of experience with
> >> the Linux NFS and RPC implementations in user space and in the
> >> kernel). ?Either we were not aware that truncation is the prevalent
> >> behavior, or we decided at the time that truncation was just as
> >> incorrect as failing outright.
> >>
> >> But we can look into it again, as I said.
> >
> > I certainly don't claim to be an expert. ?My understanding (which could
> > be flawed) is that NFS has traditionally handled this issue by
> > truncating to 16 groups. ?One can google for NFS 16 groups, and there
> > are many hits discussing this behavior. ?And truncation is obviously how
> > glibc handles it. ?If the goal is to replace the glibc implementation
> > with tirpc, then it seems rather dangerous to me to break various applications
> > that used to work under glibc...
> 
> Yes, backwards compatibility is a concern, as is compatibility with
> the existing RPC API specification (the Sun doc I referred to earlier
> in this thread).
> 
> I've checked, and a modern version of libtirpc on Solaris (a
> descendent of our libtirpc implementation, which originally came from
> Sun in the 1990's) behaves just as you describe: it truncates at NGRPS
> (16).  Existing glibc implementation aside, I take the modern Solaris
> libtirpc as a reference implementation for the RPC API.  It's likely
> this problem was fixed after our version of libtirpc forked from the
> Sun version.
> 
> I also found Jeff's patch that removes the abort() calls, dated March
> 5, 2010, in my clone of the upstream libtirpc repo.  I wonder why your
> version doesn't have this patch.
> 
> > As I mentioned, if there's doubt, let's just add both functions to the API and
> > let programmers choose. ?My guess is that most would choose the version that
> > truncates to 16 groups instead of crashing.
> 
> I don't think it's practical to introduce new APIs in this library,
> for the reasons cited above.  But perhaps we don't have to, in light
> of the fact that modern Solaris behavior agrees with what is
> implemented in glibc today.
> 
> Jeff will probably remember exactly why we did this, but I seem to
> recall that the application that was crashing didn't like the
> truncation solution either.  I could be misremembering.  We'll have to
> check in with that bug again to ensure we don't introduce a
> regression.
> 
> I'll post a more-or-less official patch for this later, and see if I
> can hook up with Jeff to find out more.
> 

Actually, I originally proposed a fix similar to what Andrew is
suggesting. Peter objected and said that returning NULL was more
correct.

The original problem was that umount.nfs was crashing (due to the
abort() calls), so we ended up fixing it to use setgroups to drop
supplimentary groups since it was not needed for umounts anyway. I also
removed those abort calls from the library.

I can see arguments for either way. On the one hand, people porting to
libtirpc are likely to be fixing code anyway -- fixing this ought to be
doable at the same time. It's really not hard to call setgroups to fix
up the groups list before you call this function.

OTOH, adopting glibc's behavioe might ease the transition, even if it
is a hack that might lead to unreliable application behavior.

-- 
Jeff Layton <jlayton@redhat.com>