[SSSD] [PATCH] sss_cache: Remove fastcache even if sssd is not running.

Simo Sorce simo at redhat.com
Wed Oct 24 15:04:27 UTC 2012


On Wed, 2012-10-24 at 15:49 +0200, Michal Židek wrote:
> https://fedorahosted.org/sssd/ticket/1584
> 
> First, I thought the race condition between sssd_nss and sss_cache 
> should be solved by some sort of file locking mechanism, but when 
> started working on it, the places where we needed to check for the file 
> being locked or free were too many and spread among monitor, nss and 
> sss_cache tool processes and it was not clear how the access is controlled.
> 
> So I decided to do it this way:
> 1. sss_cache tries to send_sighup to monitor as usual
> 2. if signal_sssd returns that sssd is not running, proceed with 
> memcache invalidation
> 3. As a part of memcache invalidation:
>      - it first opens the mc file
>      - then it checks if sssd is running (with pgrep)
>      - if sssd is running it stops the the process of invalidation
>      - if sssd is still not running it proceeds with the invalidation

Nack.

Sorry but this is not acceptable to me.
You are forking a full new process (pgrep) and that simply makes the
race condition wider not narrower.

You are moving from a simple signal to a whole setting up a new process
and pipes communication with that process which takes a lot of time
(relatively speaking).

Using a lock on the file removes the race completely and is much simpler
as it requires no communication at all.

You need to do this in only 2 places, in sss_cache and in sssd_nss right
after the open call.

Please use fcntl() with a F_SETLK and F_WRLCK on the first byte of the
cache file in sss_cache.

You do not want to wait (therefore F_SETLK instead of F_SETLCKW) in
sss_cache, if you can't get the lock it means sssd_nss is running and
you abort. You can choose to try the lock before actually sending
signals to sssd_nss and fallback to the signal only if sssd_nss is
running (lock failed).

In sssd_nss you also need to take a F_WRLCK on the first byte of the
file, however in sssd_nss case you want to retry a few times just in
case you races with sss_cache. I think retrying a couple of times
waiting a few milliseconds between each retry would be fine.

Say 3 retries waiting 50ms between each.

Make sure to handle EACESS, EAGAIN, EINTR all the same way as they all
mean the file is locked or the syscal wasn't able to get a lock for
other reasons.

> See that if sssd starts after (or during) the pgrep check (so we will 
> not catch it as running, but will assume it is off) it is not  a 
> problem, because we have file descriptor associated with file that was 
> present before sssd was running (we open the file before pgrep call). 
> sssd_nss alwas creates a new memory cache file on startup, so we will 
> only mark the old one as recycled (and not the new one), that we do not 
> care about (because it will be deleted by sssd_nss and marking that as 
> recycled is not a problem, I think -- the worst thing that can happen is 
> race between nss and cache tool while both are marking the OLD file as 
> recycled, but I can not figure out situation where this could be 
> dangerous, because there are no "temp" states that could be harmful. 

There are.

The problem is this file is single writer and sssd_nss assumes it is the
only writer, so if you invalidate the file mistakenly when sssd_nss was
active
say pgrep grepped and found nothing but before it returned the result to
your process sssd_nss started and opened the file. It is possible
because sssd_nss can be scheduled to run right after pgrep has given up
the information to send back to sss_cache to a syscall, and sssd_nss is
schedule before sss_cache can read and act on that information.

This would cause sssd_nss to not notice the file was tampered with and
keep running using the old file, which has been unlinked, so clients
wouldn't find nor use it. Performances would degrade, as the cache is
not being used until sssd is restarted.

> Both processes are changing the value of status, but both to the same 
> value).
> 
> Another thing that I like about this is that we do not have to care 
> about communication sssd vs sssd_nss vs sss_cache but only control sssd 
> vs sss_cache, which is much easier to understand.

This would be true if you used locking, the current scheme is brittle
and racy.

> Can someone see any problems that I missed?

See above.

> NOTE: The function sss_mc_set_recycled is copied from different module. 
> I did not want to make this function non static and put it to a header 
> file because it is not intended to be used directly.

This is right, thanks for not doing it.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list