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:
- sss_cache tries to send_sighup to monitor as usual
- 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.