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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-06-15 at 16:38 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

I think this is latent, and not actually reachable by current callers.

1. vcap_write_counter always returns 0
2. vcap_encode_rule can not error if vcap_val_rule is called before vcap_add_rule
3. vcap_write_rule can not error if vcap_val_rule is called before vcap_add_rule

So I propose this is fixed in seperate series.

> [ ... ]
> 
> > @@ -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?
> 

This is also true but latent again. To trigger this we would need concurrency
against the same vcap instance. Before this series vcap_mod_rule was only
called under rtnl. This series introduces a call outside rtnl, but the new LPM VCAP
is not usable by tc only the routing module, which serializes everything under
one mutex.

So I think this should be fixed in a seperate series.

> [ ... ]
> 
> 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?

Yes this becomes reachable with this series, due to calls outside rtnl.
It is a symptom of a wider issue with the per instance locking in the VCAP
api. Sashiko found another existing bug with the shared SUPER vcap registers
also caused by this, and that one is reachable in mainline, so I will send a
fix to net for the vcap locking which will also fix this problem, then send
v2 once that is settled.

> --
> pw-bot: cr