Bugzilla – Bug 1191
doubts about view and local zone locking
Last modified: 2016-12-21 11:35:52 CET
This is just from code inspection, so I may be wrong, but I suspect
locking related to views and their local zones is not sufficient in
1. in local_zones_answer(), local_zone 'z' can be used after releasing
the view lock:
(z = local_zones_lookup(view->local_zones,
qinfo->qname, qinfo->qname_len, labs,
But shouldn't we actually protect the view until we're done with
'z'? Otherwise the view's local zones can be updated via the
remote control interface in parallel. It could even be destroyed
by the view_local_zone_remove command.
2. struct acl_addr holds a pointer to struct view, and it can be
passed to local_zones_answer(), which seems to assume the passed
view is valid. This is probably the case actually today, since, as
far as I can see, views' vtree is never updated once it's built on
(re)configuration. But code comments in view.h seem to intend to
support cases where a view might even be deleted dynamically:
* For the node and name you
* need to also hold the views_tree lock to change them (or to
* delete this view) */
If that's the intent, I suspect we can't actually hold a raw
pointer to view in acl_addr; instead, for example, we'll need to
store a copy of the view name, and when using the corresponding
view acquire the views_tree lock, lookup the tree for the view by
name, use the view and then release the lock. If such a dynamic
change to views isn't intended, I'd suggest revising the code
comment so the assumption is much more clearer.
Thank you for the report.
Regarding the first item: Please correct my if I'm wrong, but I don't see an issue here. A read lock is acquired on the local zone before releasing the lock on the view. Before the the removal or type update on the view's local zone, a write lock needs to be acquired. So I think only locking the local zone should be sufficient. Adding new items to the view while the local zone is in use should not be a problem, as long as the zone in use is not altered (which should not happen since it is locked).
Regarding the second item: The comment in view.h is wrong, I removed the view deletion part. If we make view deletion possible in the future, this will indeed need some adjustment. Maybe by storing the view's name, or by storing references to the ACLs in the views.
Regarding the first point, I was probably overlooking the fact that local_zones_del_zone() acquires the zone's lock. On re-reading the code I don't see an issue either - sorry for the noise.
As for the second issue, I see that, thanks for the clarification.
Please feel free to close the report.