On Mon, 2026-08-17 at 14:13 -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.
> ---
> net: microchip: vcap: expose helpers in vcap api and update debugfs
>
> This adds two new client API helpers, vcap_val_add_rule() and
> vcap_rule_mod_action_bit(), renames VCAP_CID_PREROUTING to
> VCAP_CID_PREROUTING_L0 and adds VCAP_USER_L3. The debugfs dump learns
> about the IP4_XIP and IP6_XIP key fields, and the action-field printer
> mask expression is changed to a GENMASK() based form.
>
> A couple of questions below.
>
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > index 6e1ee15b82b7e..1ad2c44da8bd7 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > @@ -2389,6 +2389,19 @@ int vcap_add_rule(struct vcap_rule *rule)
> > }
> > EXPORT_SYMBOL_GPL(vcap_add_rule);
> >
> > +/* Validate and add rule to a VCAP instance */
> > +int vcap_val_add_rule(struct vcap_rule *rule, u16 l3_proto)
> > +{
> > + int err;
> > +
> > + err = vcap_val_rule(rule, l3_proto);
> > + if (err)
> > + return err;
> > +
> > + return vcap_add_rule(rule);
> > +}
> > +EXPORT_SYMBOL_GPL(vcap_val_add_rule);
>
> [Severity: Medium]
> This isn't a pattern introduced by this patch (every existing caller
> already does vcap_val_rule() followed by vcap_add_rule() without holding
> vctrl->lock across both), but now that it is packaged as an exported
> helper, is the check-then-act window worth documenting or closing?
>
> vcap_val_rule() ends with the capacity test and runs entirely without
> vctrl->lock:
>
> /* Finally check if there is room for the rule in the VCAP */
> return vcap_rule_space(ri->admin, ri->size);
>
> vcap_rule_space() reads the shared instance state unlocked:
>
> if (admin->last_used_addr - size < admin->first_valid_addr) {
> ...
> return -ENOSPC;
> }
>
> vcap_add_rule() then takes the mutex and the append branch of
> vcap_insert_rule() consumes address space without re-testing against
> admin->first_valid_addr:
>
> ri->addr = vcap_next_rule_addr(admin->last_used_addr, ri);
> admin->last_used_addr = ri->addr;
>
> If two adds to the same VCAP instance interleave between validation and
> insertion on a nearly full instance, can both pass the stale space check
> and the second one push last_used_addr below first_valid_addr, so the
> rule is encoded into addresses owned by another VCAP block instead of
> returning -ENOSPC?
>
> For the users present at this commit I could not construct such an
> interleaving: the L3/LPM adders added later in the series all run from an
> ordered workqueue under sparx5->router->lock, and the TC and matchall
> paths are serialized by rtnl. The one case I could not rule out is a tc
> filter installed directly into the LPM chain range concurrently with a FIB
> work item, since sparx5_tc_flower_replace() resolves the target instance
> purely from the user-supplied chain index:
>
> vrule = vcap_alloc_rule(vctrl, ndev, fco->common.chain_index,
> VCAP_USER_TC,
> fco->common.prio, 0);
>
> Is such a tc configuration rejected somewhere? If not, would holding
> vctrl->lock across validate plus insert, or re-checking vcap_rule_space()
> in the append branch of vcap_insert_rule(), be preferable?
>
Thanks, this is a good catch. That tc configuration is rejected today, but
more as an mergent side effect what has nothing to do with LPM. It is
however possible to add either goto's targeting the LPM chain, or the
matchall vcap enable rules. These will not write rule data, but the intention
is that the LPM vcap is system only.
I will add explicit checks rejecting the use of the LPM vcap from tc. I will
also add re-checking vcap_rule_space() in the append branch of
vcap_insert_rule(),
even if no caller can reach it. I will add that in patch 1, which already adds
the symmetric test to the insert-in-middle branch.
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> > b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> > index e0c65c7ab23e1..36150822ce153 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> > @@ -133,7 +135,12 @@ vcap_debugfs_show_rule_actionfield(struct vcap_control
> > *vctrl,
> > out->prf(out->dst, "%d", value[0]);
> > break;
> > case VCAP_FIELD_U32:
> > - fmsk = (1 << actionfield[action].width) - 1;
> > + if (action == VCAP_AF_MAC_LSB || action == VCAP_AF_MAC_MSB) {
> > + hex = true;
> > + break;
> > + }
> > + fmsk = actionfield[action].width ?
> > + GENMASK(actionfield[action].width - 1, 0) : 0;
> > val = *(u32 *)value;
> > out->prf(out->dst, "%u", val & fmsk);
> > break;
>
> [Severity: Low]
> This isn't a bug introduced by this patch, but should the sibling
> key-field printer get the same treatment? vcap_debugfs_show_rule_keyfield()
> in the same file still evaluates the shift first and only repairs the
> value afterwards:
>
> } else {
> u32 fmsk = (1 << keyfield[key].width) - 1;
>
> if (keyfield[key].width == 32)
> fmsk = ~0;
> out->prf(out->dst, "%u/%u", data->u32.value & fmsk,
> data->u32.mask & fmsk);
> }
>
> The printed value is correct because of the width == 32 fixup, but the
> 1 << 32 on a signed int has already been evaluated at that point, which
> looks like the same undefined shift the commit message describes fixing
> in the action-field printer.
>
> This branch is reachable today: VCAP_KF_IP_PAYLOAD_5TUPLE is
> VCAP_FIELD_U32 with width 32 in is0_normal_5tuple_ip4_keyfield for
> sparx5 and lan969x, and in the lan966x is1 keyset, and VCAP_KF_RT_FRMID
> is another W32 U32 key in lan966x is1. None of them are in the %pI4h or
> hex special cases, so dumping a rule carrying one of those keys via the
> VCAP debugfs rule dump would hit the shift with a width of 32. Would
> converting this site to the same GENMASK() form make sense while the
> area is being touched?
>
> Reported by an automated review pipeline; both items are pre-existing and
> neither is claimed by the commit message, so they may well be better as
> separate cleanups.
Yes, I will fix this case and also add the new VCAP_KF_IP4_{DIP,SIP} under the
pI4h case, same as IP4_XIP.
