Bug 1703436

Thank you for the bug report. 

We're going to look at the bug, but because there are quite a few bug reports and cases against SSSD, I wanted to ask if you have a RH subscription? If yes, it would help us to prioritize the work better, if you could also open a RH support case.. 

Coming to the technical aspect of the problem -- after a quick discussion with a colleague, currently I'm not sure what could be the issue. innetgr() is implemented in libc completely as a wrapper around setnetgrent/getnetgrent/endnetgrent (https://sourceware.org/git/?p=glibc.git;a=blob;f=inet/getnetgrent_r.c;hb=1c81d55fc4b07b51adf68558ba74ce975153e580#l451) so nss_sss does not implement innetgr on its own. So either one of setnetgrent/getnetgrent/endnetgrent  is thread-unsafe or libc itself (unlikely?). And sssd deamon is single threaded, using an event loop for pseudo-concurrency. So on the technical side, this needs more investigation, but as I said, having a support case would help prioritize the investigation better.

(In reply to Ulrich Sibiller from comment #0)
> Created attachment 1559039 [details]
> example source code
> 
> Description of problem:
> 
> "man 3 innetgr" states "The function is thread-safe." On our system it
> normally is but fails to be thread-safe if sssd is activated.
> 

I agree this sentence is in the man page nut there is the table with lists innetgr() as "MT-Unsafe race:netgrent locale" and the sentence "In the above table, netgrent in race:netgrent signifies that if any of the functions setnetgrent(), getnetgrent_r(), innetgr(), getnetgrent(), or endnetgrent() are used in parallel in different threads of a program, then data races could occur."

Additionally the glibc documentation (info libc) only says "MT-Unsafe race:netgrent locale".

I switch the component for this ticket to 'glibc' to ask the maintainers for clarification if innetgr() is expected to the thread-safe for the described use case.

bye,
Sumit

I think the code backing innetgr in glibc and sssd should be thread-safe as-is.  Unlike many other NSS functions, local state is used for it, in the result buffer.  nss_sss also uses it to keep local state across _nss_sss_setnetgrent and internal_getnetgrent_r.  See the handling of “the struct __netgrent *result” argument.

I believe the race:netgrent annotation for innetgr is a bug.  The glibc manual even lists thread safety as a benefit of innetgr:

     This function tests whether the triple specified by the parameters
     HOST, USER, and DOMAIN is part of the netgroup NETGROUP.  Using
     this function has the advantage that

       1. no other netgroup function can use the global netgroup state
          since internal locking is used and
       2. the function is implemented more efficiently than successive
          calls to the other ‘set’/‘get’/‘endnetgrent’ functions.

I think we need to find out why innetgr is not thread-safe with sssd and then re-evaluate whether we should fix this issue, and update the glibc manual accordingly.  I would appreciate if you could help with that since I'm not familiar with sssd internals.  Thanks.

Thank you Florian for the details.

I will move the ticket back to the SSSD component.

I guess the issue might be caused by the way concurrent netgroup requests are handled by the SSSD NSS responder process. Can you explain 

"""

       1. no other netgroup function can use the global netgroup state
          since internal locking is used and
       2. the function is implemented more efficiently than successive
          calls to the other ‘set’/‘get’/‘endnetgrent’ functions.
""""
with some more details? I wonder if nss_sss or SSSD's NSS responder can detect somehow that the original call is innetgr() and not a sequence of ‘set’/‘get’/‘endnetgrent’ functions. But after a short look at the code in inet/getnetgrent_r.c it looks to me that innetgr() is still implemented as a sequence of set/multiple gets/end calls and I also didn't see any special locking for innetgr() only for the individual ‘set’/‘get’/‘endnetgrent’ functions.

bye,
Sumit

I think you can detect the call sequence using the struct __netgrent * argument.  See the nss/nss_files/files-netgrp.c code in glibc.

(In reply to Florian Weimer from comment #6)
> I think you can detect the call sequence using the struct __netgrent *
> argument.  See the nss/nss_files/files-netgrp.c code in glibc.

Ok, I see. How should NSS modules outside of the glibc source tree use struct __netgrent, since it is not defined in a public header. Currently SSSD and e.g. nss-pam-ldapd are using a copy of the struct definition in their own source tree. But this might be risky, since glibc is free to change in internals of the struct at will (see e.g. https://pagure.io/SSSD/sssd/issue/3993 for a similar case).

I will try to document this better on the glibc side. I don't think there is a way around defining struct __netgrent in a public header file.

The sssd sources already contain a copy of the definition of struct __netgrent because it is impossible to implement netgroup support without it. (The first few members are used for passing data to the glibc NSS framework.) I think you can reuse the data member to store a pointer to your own internal data.  You must set the data member to NULL in the endnetgrent function and free the resources there.

(In reply to Florian Weimer from comment #8)
> I will try to document this better on the glibc side. I don't think there is
> a way around defining struct __netgrent in a public header file.
> 
> The sssd sources already contain a copy of the definition of struct
> __netgrent because it is impossible to implement netgroup support without
> it. (The first few members are used for passing data to the glibc NSS
> framework.) I think you can reuse the data member to store a pointer to your
> own internal data.  You must set the data member to NULL in the endnetgrent
> function and free the resources there.

Hi Florian,

thank you for taking care of this in glibc.

I will check from the SSSD side how we can use the data member from __netgrent to make netgroup calls thread safe.

bye,
Sumit

Upstream ticket:
https://github.com/SSSD/sssd/issues/5540

Pushed PR: https://github.com/SSSD/sssd/pull/5541

* `master`
    * 29abf94e3aec2c54c76adf61318099097c41ea77 - intg test: test is innetgr() is thread-safe
    * 88eec1c22f188adc49f41b81fe5af03c995c690d - nss client: make innetgr() thread safe

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (sssd bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2021:4435