| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ab08efd8-3123-7560-0ef0-036dc156db9f@intel.com>
Date: Fri, 21 Apr 2023 16:22:52 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<alexandr.lobakin@...el.com>, <david.m.ertman@...el.com>,
<michal.swiatkowski@...ux.intel.com>,
<marcin.szycik@...ux.intel.com>, <pawel.chmielewski@...el.com>,
<sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next 06/12] ice: Add guard rule when creating FDB in
switchdev
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:06 +0200
> From: Marcin Szycik <marcin.szycik@...el.com>
>
> Introduce new "guard" rule upon FDB entry creation.
>
> It matches on src_mac, has valid bit unset, allow_pass_l2 set
> and has a nop action.
[...]
> +static struct ice_rule_query_data *
> +ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
> + const unsigned char *mac)
> +{
> + struct ice_adv_rule_info rule_info = { 0 };
> + struct ice_rule_query_data *rule;
> + struct ice_adv_lkup_elem *list;
> + const u16 lkups_cnt = 1;
> + int err;
You can initialize it with -%ENOMEM right here in order to...
> +
> + rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> + if (!rule) {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> +
> + list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
> + if (!list) {
> + err = -ENOMEM;
> + goto err_list_alloc;
> + }
...make those 2 ifs goto-oneliners :3 As...
> +
> + list[0].type = ICE_MAC_OFOS;
> + ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac);
> + eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr);
> +
> + rule_info.allow_pass_l2 = true;
> + rule_info.sw_act.vsi_handle = vsi_idx;
> + rule_info.sw_act.fltr_act = ICE_NOP;
> + rule_info.priority = 5;
> +
> + err = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, rule);
...it's overwritten here anyway, so it is safe to init it with an error
value.
> + if (err)
> + goto err_add_rule;
> +
> + return rule;
> +
> +err_add_rule:
> + kfree(list);
> +err_list_alloc:
> + kfree(rule);
> +err_exit:
> + return ERR_PTR(err);
> +}
> +
> static struct ice_esw_br_flow *
> ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
> int port_type, const unsigned char *mac)
> {
> - struct ice_rule_query_data *fwd_rule;
> + struct ice_rule_query_data *fwd_rule, *guard_rule;
> struct ice_esw_br_flow *flow;
> int err;
>
> @@ -155,10 +202,22 @@ ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
> goto err_fwd_rule;
> }
>
> + guard_rule = ice_eswitch_br_guard_rule_create(hw, vsi_idx, mac);
> + if (IS_ERR(guard_rule)) {
> + err = PTR_ERR(guard_rule);
Aaah ok, that's what you meant in the previous mails. I see now.
You can either leave it like that or there's an alternative -- pick the
one that you like the most:
guard_rule = ice_eswitch_...
err = PTR_ERR(guard_rule);
if (err) {
...
> + dev_err(dev, "Failed to create eswitch bridge %sgress guard rule, err: %d\n",
> + port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> + err);
You still can print it via "%pe" + @guard_rule instead of @err :p (same
with @fwd_rule above)
> + goto err_guard_rule;
> + }
> +
> flow->fwd_rule = fwd_rule;
> + flow->guard_rule = guard_rule;
>
> return flow;
[...]
> @@ -4624,7 +4628,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> */
> static u16
> ice_find_recp(struct ice_hw *hw, struct ice_prot_lkup_ext *lkup_exts,
> - enum ice_sw_tunnel_type tun_type)
> + struct ice_adv_rule_info *rinfo)
Can be const I think?
> {
> bool refresh_required = true;
> struct ice_sw_recipe *recp;
[...]
> @@ -5075,6 +5082,14 @@ ice_add_sw_recipe(struct ice_hw *hw, struct ice_sw_recipe *rm,
> set_bit(buf[recps].recipe_indx,
> (unsigned long *)buf[recps].recipe_bitmap);
> buf[recps].content.act_ctrl_fwd_priority = rm->priority;
> +
> + if (rm->need_pass_l2)
> + buf[recps].content.act_ctrl |=
> + ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
> +
> + if (rm->allow_pass_l2)
> + buf[recps].content.act_ctrl |=
> + ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;
I don't like these line breaks :s
type_of_content *cont;
...
/* As far as I can see, it can be used above as well */
cont = &buf[recps].content;
if (rm->need_pass_l2)
cont->act_ctrl |= ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
if (rm->allow_pass_l2)
cont->act_ctrl |= ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;
> recps++;
> }
>
[...]
> @@ -6166,6 +6190,11 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> act |= ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_DROP |
> ICE_SINGLE_ACT_VALID_BIT;
> break;
> + case ICE_NOP:
> + act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
> + ICE_SINGLE_ACT_VSI_ID_S) & ICE_SINGLE_ACT_VSI_ID_M;
`FIELD_PREP(ICE_SINGLE_ACT_VSI_ID_M, rinfo->sw_act.fwd_id.hw_vsi_id)`?
> + act &= ~ICE_SINGLE_ACT_VALID_BIT;
> + break;
> default:
> status = -EIO;
> goto err_ice_add_adv_rule;
> @@ -6446,7 +6475,7 @@ ice_rem_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> return -EIO;
> }
>
> - rid = ice_find_recp(hw, &lkup_exts, rinfo->tun_type);
> + rid = ice_find_recp(hw, &lkup_exts, rinfo);
> /* If did not find a recipe that match the existing criteria */
> if (rid == ICE_MAX_NUM_RECIPES)
> return -EINVAL;
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> index c84b56fe84a5..5ecce39cf1f5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -191,6 +191,8 @@ struct ice_adv_rule_info {
> u16 vlan_type;
> u16 fltr_rule_id;
> u32 priority;
> + u8 need_pass_l2;
> + u8 allow_pass_l2;
They can be either true or false, nothing else, right? I'd make them
occupy 1 bit per var then:
u16 need_pass_l2:1;
u16 allow_pass_l2:1;
u16 src_vsi;
+14 free bits for more flags, no holes (stacked with ::src_vsi).
> u16 src_vsi;
> struct ice_sw_act_ctrl sw_act;
> struct ice_adv_rule_flags_info flags_info;
> @@ -254,6 +256,9 @@ struct ice_sw_recipe {
> */
> u8 priority;
>
> + u8 need_pass_l2;
> + u8 allow_pass_l2;
(same with bitfields here, just use u8 :1 instead of u16 here to stack
with ::priority)
> +
> struct list_head rg_list;
[...]
Thanks,
Olek
Powered by blists - more mailing lists