| 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: <MW4PR11MB57760F836B7714BC22A26FC7FD639@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Thu, 20 Apr 2023 11:27:11 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Ertman, David M" <david.m.ertman@...el.com>,
"michal.swiatkowski@...ux.intel.com"
<michal.swiatkowski@...ux.intel.com>,
"marcin.szycik@...ux.intel.com" <marcin.szycik@...ux.intel.com>,
"Chmielewski, Pawel" <pawel.chmielewski@...el.com>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next 05/12] ice: Switchdev FDB events support
> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: środa, 19 kwietnia 2023 17:39
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Lobakin, Aleksander <aleksander.lobakin@...el.com>; Ertman, David M
> <david.m.ertman@...el.com>; michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel
> <pawel.chmielewski@...el.com>; Samudrala, Sridhar <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().
It will be useful later, with vlans support lkups_cnt will be equal to 1 or 2.
Can we keep it as it is?
>
> > + 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...
This is really cool, but I think it won't work here. I need to keep err in order to
return it in the err flow. I can't use fwd_rule for this purpose because
return type is ice_esw_br_flow not ice_rule_query_data.
>
> > + }
> > +
> > + flow->fwd_rule = fwd_rule;
> > +
> > + return flow;
> > +
> > +err_fwd_rule:
> > + kfree(flow);
> > +
> > + return ERR_PTR(err);
>
> ...you can return @fwd_rule directly.
>
I can't return @fwd_rule here because return type is different
This function is meant to return @flow.
[...]
> > +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.
I can make it in to one line now, thanks.
>
> > + 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