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
