On Friday, January 08, 2016 09:52:38 AM Huw Davies wrote:
> The reason is to allow different labelling protocols for
> different address families with the same domain.
>
> This requires the addition of an address family attribute
> in the netlink communication protocol. It is used in several
> messages:
>
> NLBL_MGMT_C_ADD and NLBL_MGMT_C_ADDDEF take it as an optional
> attribute for the unlabelled protocol. It may be one of AF_INET,
> AF_INET6 and AF_UNSPEC (to specify both address families). If is
> it missing, it defaults to AF_UNSPEC.
>
> NLBL_MGMT_C_LISTALL and NLBL_MGMT_C_LISTDEF return it as part of
> the enumeration of each item. Addtionally, it may be sent to
> LISTDEF to specify which address family to return.
>
> Signed-off-by: Huw Davies <h...@codeweavers.com>
...
> -static struct netlbl_dom_map *netlbl_domhsh_search_def(const char *domain)
> +static struct netlbl_dom_map *netlbl_domhsh_search_def(const char *domain,
> + u16 family)
> {
> struct netlbl_dom_map *entry;
>
> - entry = netlbl_domhsh_search(domain);
> + entry = netlbl_domhsh_search(domain, family);
> if (entry == NULL) {
> - entry = netlbl_domhsh_rcu_deref(netlbl_domhsh_def);
> - if (entry != NULL && !entry->valid)
> - entry = NULL;
> + if (family == AF_INET || family == AF_UNSPEC) {
> + entry = netlbl_domhsh_rcu_deref(netlbl_domhsh_def_ipv4);
> + if (entry != NULL && !entry->valid)
> + entry = NULL;
If entry is non-NULL and valid, why not return immediately?
> + }
> + if (entry == NULL &&
> + (family == AF_INET6 || family == AF_UNSPEC)) {
> + entry = netlbl_domhsh_rcu_deref(netlbl_domhsh_def_ipv6);
> + if (entry != NULL && !entry->valid)
> + entry = NULL;
> + }
> }
...
> @@ -264,13 +285,19 @@ static int netlbl_domhsh_validate(const struct
> netlbl_dom_map *entry) if (entry == NULL)
> return -EINVAL;
>
> + if (entry->family != AF_INET && entry->family != AF_INET6)
> + if (entry->def.type != NETLBL_NLTYPE_UNLABELED ||
> + entry->family != AF_UNSPEC)
> + return -EINVAL;
There really is no need for a nested if-then in the above code, granted the
combined conditional would be a bit more complex, but I would rather see that
than this.
> @@ -385,9 +415,10 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
> rcu_read_lock();
> spin_lock(&netlbl_domhsh_lock);
> if (entry->domain != NULL)
> - entry_old = netlbl_domhsh_search(entry->domain);
> + entry_old = netlbl_domhsh_search(entry->domain, entry->family);
> else
> - entry_old = netlbl_domhsh_search_def(entry->domain);
> + entry_old = netlbl_domhsh_search_def(entry->domain,
> + entry->family);
> if (entry_old == NULL) {
> entry->valid = 1;
>
> @@ -397,7 +428,37 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
> &rcu_dereference(netlbl_domhsh)->tbl[bkt]);
> } else {
> INIT_LIST_HEAD(&entry->list);
> - rcu_assign_pointer(netlbl_domhsh_def, entry);
> + switch (entry->family) {
> + struct netlbl_dom_map *entry2;
I'm not a fan of declaring variable here, move it up to the top of the
function, or if you must, make it local to the AF_UNSPEC case below. Also,
while I'm being nit-picky, I dislike numbers in variable names unless we are
dealing with something that honestly uses numbers as part of the name, e.g.
ipv6, rename "entry2" to "entry_b" (or something along those lines) please.
> + case AF_INET:
> + rcu_assign_pointer(netlbl_domhsh_def_ipv4,
> + entry);
> + break;
> + case AF_INET6:
> + rcu_assign_pointer(netlbl_domhsh_def_ipv6,
> + entry);
> + break;
> + case AF_UNSPEC:
> + if (entry->def.type != NETLBL_NLTYPE_UNLABELED)
> + return -EINVAL;
> + entry2 = kzalloc(sizeof(*entry2), GFP_ATOMIC);
> + if (!entry2)
> + return -ENOMEM;
> + entry2->family = AF_INET6;
> + entry2->def.type = NETLBL_NLTYPE_UNLABELED;
> + entry2->valid = 1;
> + entry->family = AF_INET;
> + rcu_assign_pointer(netlbl_domhsh_def_ipv4,
> + entry);
> + rcu_assign_pointer(netlbl_domhsh_def_ipv6,
> + entry2);
> + break;
> + default:
> + /* Already checked in
> + * netlbl_domhsh_validate()
> + */
Another style point - unless were talking about function header comment
blocks, don't place a multi-line comment terminator on a separate line. For
example, instead of the above, so something like this:
/* already checked in
* netlbl_domhsh_validate() */
> + return -EINVAL;
> + }
> }
--
paul moore
security @ redhat
