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

Jan Zelený jzeleny at redhat.com
Tue Jan 10 09:49:41 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.

Although it's very unlikely scenario, I agree that we always choose not to 
leave anything to chance in similar scenarios. Is there any downside to moving 
the code?

> > 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.

I agree

> > > 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.

I thought we've already done that in several parts of the code, haven't we?


I also have one cosmetic proposal for patch #0009:

1) On line 452 use MC_ALIGN64 instead of (n_elem + 7) & (~7)

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


More information about the sssd-devel mailing list