Re: [PATCH net-next 1/9] net: microchip: vcap: fix rule move for rules of coprime size

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: microchip: vcap: fix rule move for rules of coprime size

This patch addresses a discrepancy in how software and hardware move rule
addresses on insert and delete for the LPM VCAP. It introduces a uniform
offset computation aligned to the least common multiple of all rule sizes
in a block. This ensures that hardware and software representations remain
synchronized and all rules land on correctly aligned addresses.

> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index 0fdb5e363badc..6946fd7384581 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c

[ ... ]

> @@ -2275,8 +2336,7 @@ int vcap_add_rule(struct vcap_rule *rule)
>  		       __func__, __LINE__, ret);
>  		goto out;
>  	}
> -	if (move.count > 0)
> -		vcap_move_rules(ri, &move);
> +	vcap_move_rules(ri, &move);
>  
>  	/* Set the counter to zero */
>  	ret = vcap_write_counter(ri, &ctr);

[Severity: High]
This is a pre-existing issue, but does vcap_add_rule() leak the newly
duplicated rule if operations like vcap_write_counter() or
vcap_write_rule() fail later on?

If a failure occurs, the code takes the goto out path:

vcap_add_rule() {
    ...
    ret = vcap_write_rule(ri);
    if (ret) {
        pr_err("%s:%d: rule write error: %d\n", __func__, __LINE__, ret);
        goto out;
    }
out:
    mutex_unlock(&ri->admin->lock);
    return ret;
}

This seems to exit without reverting the earlier list insertions and
hardware moves.

Could this leave the software and hardware states permanently inconsistent
and leak the newly duplicated rule?

[ ... ]

> @@ -2488,59 +2548,52 @@ int vcap_mod_rule(struct vcap_rule *rule)
>  }
>  EXPORT_SYMBOL_GPL(vcap_mod_rule);

[Severity: High]
This isn't a bug introduced by this patch, but does vcap_mod_rule() discard
the active software rule pointer and overwrite hardware with stale addresses?

Looking at vcap_mod_rule(), the function validates the rule using
vcap_get_locked_rule() but ignores the returned pointer:

vcap_mod_rule(struct vcap_rule *rule) {
    struct vcap_rule_internal *ri = to_intrule(rule);
    ...
    if (!vcap_get_locked_rule(ri->vctrl, ri->data.id))
        return -ENOENT;
    ...
    err = vcap_encode_rule(ri);
    if (err)
        goto out;

    err = vcap_write_rule(ri);
}

Because the function continues to use the caller provided copy ri, it
doesn't see updates to the real rule in the admin->rules list.

If the real rule's hardware address was previously shifted due to insertions
or deletions, could writing to the stale address in ri->addr corrupt other
rules in the hardware VCAP?

[ ... ]

One other pre-existing issue noticed while reviewing this subsystem:

[Severity: High]
This is a pre-existing issue, but is it safe to iterate over the admin->rules
list across different VCAP instances without acquiring admin->lock for
each instance?

In vcap_rule_exists():

static bool vcap_rule_exists(struct vcap_control *vctrl, u32 id) {
    ...
    list_for_each_entry(admin, &vctrl->list, list)
        list_for_each_entry(ri, &admin->rules, list)
            if (ri->data.id == id)
                return true;
    return false;
}

If another thread is concurrently inserting or deleting a rule on another
VCAP instance while holding that instance's lock, could this unprotected
concurrent access encounter a data race and dereference a poisoned pointer?
-- 
pw-bot: cr