[SSSD] [PATCH] mmap_cache: Check if slot and name_ptr are not invalid.

Jakub Hrozek jhrozek at redhat.com
Sun Aug 11 17:35:48 UTC 2013


On Wed, Aug 07, 2013 at 06:38:47PM +0200, Michal Židek wrote:
> On 08/05/2013 09:30 PM, Simo Sorce wrote:
> > On Mon, 2013-08-05 at 21:13 +0200, Michal Židek wrote:
> >> This is very ugly hotfix for ticket:
> >> https://fedorahosted.org/sssd/ticket/2018
> >>
> >> So far we were not able to find out why the slot or name_ptr
> >> values were corrupted, but this should at least prevent segfaults.
> >
> > NACK, (but close)
> >
> > Please do not add all those comments about removing checks, those checks
> > are good to stay forever, the cache may be corrupted by a bad disk
> > sector or whatever, so they should never be removed.
> 
> Ok. I left just one FIXME (actually two, but related) label in the case
> where this patch relies on the same offset values of strs in both
> group and passwd data structure. I do not expect these structures to
> be changed, but it is something we want to have at least noticed in
> the code.
> 
> >
> > Also please do not just return ENOENT, and pretend nothing happened.
> > If the cache is corrupted we want to take immediate corrective action.
> > I suggest we return a new SSSD error and in the topmost nsscache caller
> > we invalidate the current cache and reinit.
> 
> Actually I started doing it like this, but handling the problem in the
> topmost caller is not very good idea here. When I was writing it
> I found several topmost callers and even forcing the error
> to bubble up to these callers required some api changes. In addition
> we would have to make sure we handle the corrupted memory cache case
> in the future, when we use the affected functions in other topmost
> callers (it is not so easy to track). So I decided to handle it
> on the place where we detect the error.
> 
> >
> > Optional (may be should be a separate ticket)
> > We currently reinit by closing the file and creating a new one. We
> > should not do that, we should rather reset the header and then just zero
> > all the data and reinitialize all the various areas. The reason is that
> > if we have some bad bug and we keep creating new files we might run out
> > of space because clients may have old files open and mmapped, and unless
> > they perform a new operations, they may not release them. Note we should
> > *not* use ftruncate() here, just write the appropriate initialization
> > values everywhere needed.
> 
> I agree, but sss_mmap_cache_reinit is completely unsuitable for this
> case and a huge overkill, even if we might change it in the future.
> I created a new function sss_mmap_cache_reset which is very
> lightweight. I will not create it as a separate ticket
> since I do not want to use the reinit function even temporary for
> this case.
> 
> New patch is attached.
> 
> Thanks
> Michal
> 
> 
> 

I like the patch. I asked Simo to take a look, too, since he wrote the
memcache support:

18:54 < jhrozek> anyway, the subject is "[PATCH] mmap_cache: Check if slot and name_ptr are not invalid."
18:58 < simo> ok it does look ok
18:59 < simo> what he describes as "I won't do this or that" is actually what I wanted him to do anyway :-D
18:59 < simo> mzidek: good job
18:59 < simo> (if it passes some testing :-)
19:00 < jhrozek> simo: I've been running with the patch applied today on my laptop
19:00 < simo> jhrozek: it would be nice to have a test binary that can randomly 'reset' the cache and another binary that loops over the cache
19:00 < simo> just to make sure these resets will not cause issues in sss_nss
19:00 < simo> they shouldn't but ...
19:00 < simo> jhrozek: yes but it is all error paths
19:00 < simo> it is very unlikely you excercised that code at all
19:01 < jhrozek> yes, but at least we know the code is sane
19:01 < simo> nope we don't
19:01 < simo> we only know it doesn;t break the build
19:01 < jhrozek> ok, I will do more testing with sss_cache thrown in to invalidate the cache etc
19:01 < simo> but if it is never called we do not know that it works
19:02 < jhrozek> we know the usual paths work and master won't be horribly broken with the patch
19:02 < simo> sss_cache will not do that kind of invalidation
19:02 < simo> jhrozek: oh I am not saying we should hold the patch

So Ack from me.

Really good work from both you and Lukas. The test Simo proposed is tracked
by https://fedorahosted.org/sssd/ticket/2045



More information about the sssd-devel mailing list