| 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
| ||
|
Date: Wed, 19 Apr 2023 17:38:53 +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 05/12] ice: Switchdev FDB events support
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:05 +0200
> Listen for SWITCHDEV_FDB_{ADD|DEL}_TO_DEVICE events while in switchdev
> mode. Accept these events on both uplink and VF PR ports. Add HW
> rules in newly created workqueue. FDB entries are stored in rhashtable
> for lookup when removing the entry and in the list for cleanup
> purpose. Direction of the HW rule depends on the type of the ports
> on which the FDB event was received:
[...]
> +static int
> +ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule)
> +{
> + int err;
> +
> + if (!rule)
> + return -EINVAL;
> +
> + err = ice_rem_adv_rule_by_id(hw, rule);
> + kfree(rule);
> +
> + return err;
> +}
> +
> +static struct ice_rule_query_data *
> +ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, u16 vsi_idx, int port_type,
(no types shorter than u32 on the stack reminder)
> + const unsigned char *mac)
> +{
> + struct ice_adv_rule_info rule_info = { 0 };
> + struct ice_rule_query_data *rule;
> + struct ice_adv_lkup_elem *list;
> + u16 lkups_cnt = 1;
Why have it as variable if it doesn't change? Just embed it into the
ice_add_adv_rule() call and replace kcalloc() with kzalloc().
> + int err;
> +
> + rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> + if (!rule)
> + return ERR_PTR(-ENOMEM);
> +
> + list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
[...]
> + fwd_rule = ice_eswitch_br_fwd_rule_create(hw, vsi_idx, port_type, mac);
> + if (IS_ERR(fwd_rule)) {
> + err = PTR_ERR(fwd_rule);
> + dev_err(dev, "Failed to create eswitch bridge %sgress forward rule, err: %d\n",
> + port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> + err);
> + goto err_fwd_rule;
A bit suboptimal. To print errno pointer, you have %pe modifier, so you
can just print err as:
... forward rule, err: %pe\n", ... : "in", fwd_rule);
Then you don't need @err at all and then below...
> + }
> +
> + flow->fwd_rule = fwd_rule;
> +
> + return flow;
> +
> +err_fwd_rule:
> + kfree(flow);
> +
> + return ERR_PTR(err);
...you can return @fwd_rule directly.
> +}
> +
> +static struct ice_esw_br_fdb_entry *
> +ice_eswitch_br_fdb_find(struct ice_esw_br *bridge, const unsigned char *mac,
> + u16 vid)
> +{
> + struct ice_esw_br_fdb_data data = {};
(nit: assign `vid` here)
> +
> + ether_addr_copy(data.addr, mac);
> + data.vid = vid;
> + return rhashtable_lookup_fast(&bridge->fdb_ht, &data,
> + ice_fdb_ht_params);
> +}
[...]
> +static void
> +ice_eswitch_br_fdb_offload_notify(struct net_device *dev,
> + const unsigned char *mac, u16 vid,
> + unsigned long val)
> +{
> + struct switchdev_notifier_fdb_info fdb_info;
> +
> + fdb_info.addr = mac;
> + fdb_info.vid = vid;
> + fdb_info.offloaded = true;
(same for all of them. Declare-time initializer is faster BTW)
> + call_switchdev_notifiers(val, dev, &fdb_info.info, NULL);
> +}
[...]
> +static int
> +ice_eswitch_br_switchdev_event(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> + struct ice_esw_br_offloads *br_offloads =
> + ice_nb_to_br_offloads(nb, switchdev_nb);
> + struct netlink_ext_ack *extack =
> + switchdev_notifier_info_to_extack(ptr);
(initialize-later-to-avoid-line-breaks?)
> + struct switchdev_notifier_fdb_info *fdb_info;
> + struct switchdev_notifier_info *info = ptr;
> + struct ice_esw_br_fdb_work *work;
> + struct net_device *upper;
> + struct ice_esw_br_port *br_port;
RCT :s
> +
> + upper = netdev_master_upper_dev_get_rcu(dev);
> + if (!upper)
> + return NOTIFY_DONE;
> +
> + if (!netif_is_bridge_master(upper))
> + return NOTIFY_DONE;
> +
> + if (!ice_eswitch_br_is_dev_valid(dev))
> + return NOTIFY_DONE;
> +
> + br_port = ice_eswitch_br_netdev_to_port(dev);
> + if (!br_port)
> + return NOTIFY_DONE;
> +
> + switch (event) {
> + case SWITCHDEV_FDB_ADD_TO_DEVICE:
> + case SWITCHDEV_FDB_DEL_TO_DEVICE:
> + fdb_info = container_of(info,
> + struct switchdev_notifier_fdb_info,
Nit: `typeof(*fdb_info)` is shorter and would probably fit into the prev
line.
> + info);
> +
> + work = ice_eswitch_br_fdb_work_alloc(fdb_info, br_port, dev,
> + event);
[...]
> +enum {
> + ICE_ESWITCH_BR_FDB_ADDED_BY_USER = BIT(0),
> +};
> +
> +struct ice_esw_br_fdb_entry {
> + struct ice_esw_br_fdb_data data;
> + struct rhash_head ht_node;
> + struct list_head list;
> +
> + int flags;
They can't be negative I believe? u32 then? Also I think here's a 4-byte
hole :s But since all of the members here except this one are 8-byte
aligned, you can't avoid it (can be filled anytime later with some other
<= 4-byte field)
> +
> + struct net_device *dev;
> + struct ice_esw_br_port *br_port;
> + struct ice_esw_br_flow *flow;
> +};
[...]
Thanks,
Olek
Powered by blists - more mailing lists