[SSSD] [PATCH 0/4]: Actual memory cache implementation

Stephen Gallagher sgallagh at redhat.com
Tue Jan 10 01:29:45 UTC 2012


On Mon, 2012-01-09 at 18:24 -0500, Simo Sorce wrote:
> On Mon, 2012-01-09 at 16:30 -0500, Stephen Gallagher wrote:
> > On Tue, 2012-01-03 at 18:00 -0500, Simo Sorce wrote:
> > > >From [PATCH 0/0] A shared memory cache to perform better:
> > > 
> > > 0/4: Actual memory cache implementation
> > > These is the bulk of the work, these patches are still a bit rough at
> > > the edges, grep for FIXMEs and TODOs and you'll see some plumbing (for
> > > example configure options in sssd to set expiration time and cache sizes
> > > are missing and are still harcoded).
> > 
> > 
> > 
> > Patch 0008: Ack
> > 
> > Patch 0009: Nack
> > 
> > As the fixmes say, please parameterize the cache sizes and timeout
> > period.
> > 
> > You have a time-of-check/time-of-use bug with the stat of mc_ctx->file.
> > Please open it first and then use fstat() instead, to avoid a
> > race-condition exploit.
> 
> Not sure what race condition is there.
> 
> sssd_nss is the only thing that can create/manipulate that file, in what
> case do you see a race condition ?
> 

If filesystem protections are ever incorrect, then you have a situation
where theoretically a user could do something like replace the mmap file
with a symlink to /etc/passwd in the short gap between the stat() check
and the file open. We would then fail to validate the file and promptly
destroy it, hosing the system.

> The problem of using fstat() is that it unnecessarily(IMO) complicates
> the code for this case.
> 

I think it's a necessary complication. Please use it.

> > Please use errno_t for the return code of functions that return errno
> > values.
> 
> Ok.
> 
> > I think the check_header pieces are unnecessary. When the NSS provider
> > is started, it should always remove an existing fast cache. This will
> > make the behavior better match users' expectations.
> 
> Not sure about this, it unnecessarily causes cache lookups to go though
> the pipe to re-populate the fast cache. Perhaps we need to discuss a bit
> more pros-cons of either approach.
> 

Sure, we can talk about it. I'm looking at it from the users'
perspectives, who I think would generally expect (and be alright with)
the fast cache being emptied on service restart. Since we still have the
not-quite-as-fast persistent LDB cache, I think the gain isn't worth the
user confusion.

> > Don't use ftruncate() to generate the cache file. It's not safe on all
> > platforms or filesystems. Probably better to use fseek() (even though
> > it's a bit slower. It's a rare operation).
> 
> I asked our filesystem gurus and they think ftruncate() is the best way
> to go. (and the kernel sources confirm it works with every local file
> systems even VFAT :-)
> 

Well, my concern was more with porting to other platforms, but I suppose
we can burn those bridges when we come to them.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120109/03a52138/attachment.sig>


More information about the sssd-devel mailing list