<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 1 May 2021 at 15:51, D. Hugh Redelmeier <<a href="mailto:hugh@mimosa.com">hugh@mimosa.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The only place where I notice a refcnt_t manipulated without a lock is<br>
in free_root_certs.<br>
<br></blockquote><div><br></div><div>Oops, yea.  I don't think it is a problem (but could certainly do with more notes).  free_root_certs() is called:</div><div><div><br></div><div>- during shutdown and after all the crypto has been cleaned up</div><div>-> there should be no helper threads</div><div>-> there should be no other references</div><div>if this isn't the case then, oops, stumble on - we're going down</div><div><br></div><div>- when pluto is idle (as in no request for root certs has happened for 5 minutes)</div><div>-> having a structure sit on a reference for 5 minutes is a bug (or I'm debugging a crypto thread :-)</div><div><br></div><div>In the latter case, this kicks in:</div><div></div></div><div><br></div><div>        /*<br>         * Deal with a helper thread being stuck (because it is being debugged?);<br>         * need to peek at the refcnt.<br>         */<br>        if (root_cert_db->refcnt.count > 1) {<br>                llog(RC_LOG, logger, "root certs still in use; suspect stuck thread");<br>                /* extend or set cert cache lifetime */<br>                schedule_oneshot_timer(EVENT_FREE_ROOT_CERTS, FREE_ROOT_CERTS_TIMEOUT);<br>                return;<br>        }<br></div><div><br></div><div>Should the race you describe occur, then the next free_root-certs() call in 5 minutes will fix things up.</div><div><br></div><div>With that in mind, all I think is needed is a function returning the refcnt, or even just return that more than one thing is using the object.</div><div><br></div><div>(sometimes I wonder if refcnt and the memory allocator should be merged)</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
One problem is that the refcount can change between being observed and<br>
the action that is affected by the observation.<br>
<br>
Another is that the actual refcnt member isn't guaranteed to be<br>
atomic.  If all operations were serialized by the mutex, this would<br>
not be a problem.<br>
<br>
The unprotected observation is a test to see if there will be any<br>
remaining references after this reference is deleted.<br>
<br>
It would be simple to change root_certs_delref() to safely return the<br>
necessary information.  I've actually started to do that.<br>
<br>
But it is a little bit awkward that the existing code does not delete<br>
the reference if the count was larger than one.  My simple change does<br>
delete the reference.<br>
<br>
I guess that the brute-force fix would be to add a function<br>
"refcnt_delref_if_only".  It doesn't feel optimal.<br>
<br>
What's the right fix?<br>
_______________________________________________<br>
Swan-dev mailing list<br>
<a href="mailto:Swan-dev@lists.libreswan.org" target="_blank">Swan-dev@lists.libreswan.org</a><br>
<a href="https://lists.libreswan.org/mailman/listinfo/swan-dev" rel="noreferrer" target="_blank">https://lists.libreswan.org/mailman/listinfo/swan-dev</a><br>
</blockquote></div></div>