| 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: Tue, 06 Jan 2015 07:31:25 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Scott Feldman <sfeldma@...il.com>
CC: Thomas Graf <tgraf@...g.ch>,
Jiří Pírko <jiri@...nulli.us>,
Jamal Hadi Salim <jhs@...atatu.com>,
"simon.horman@...ronome.com" <simon.horman@...ronome.com>,
Netdev <netdev@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [net-next PATCH v1 05/11] net: rocker: add set flow rules
On 01/05/2015 11:23 PM, Scott Feldman wrote:
> On Wed, Dec 31, 2014 at 11:47 AM, John Fastabend
> <john.fastabend@...il.com> wrote:
>> Implement set flow operations for existing rocker tables.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
[...]
>> +static int is_valid_net_flow_action(struct net_flow_action *a, int *actions)
>> +{
>> + int i;
>> +
>> + for (i = 0; actions[i]; i++) {
>> + if (actions[i] == a->uid)
>> + return is_valid_net_flow_action_arg(a, a->uid);
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +static int is_valid_net_flow_match(struct net_flow_field_ref *f,
>> + struct net_flow_field_ref *fields)
>> +{
>> + int i;
>> +
>> + for (i = 0; fields[i].header; i++) {
>> + if (f->header == fields[i].header &&
>> + f->field == fields[i].field)
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +int is_valid_net_flow(struct net_flow_table *table, struct net_flow_flow *flow)
>> +{
>> + struct net_flow_field_ref *fields = table->matches;
>> + int *actions = table->actions;
>> + int i, err;
>> +
>> + for (i = 0; flow->actions[i].uid; i++) {
>> + err = is_valid_net_flow_action(&flow->actions[i], actions);
>> + if (err)
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; flow->matches[i].header; i++) {
>> + err = is_valid_net_flow_match(&flow->matches[i], fields);
>> + if (err)
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>
> All the above doesn't look rocker-specific...up-level?
>
Yes, already in the works for v2.
>> +
>> +static u32 rocker_goto_value(u32 id)
>> +{
>> + switch (id) {
>> + case ROCKER_FLOW_TABLE_ID_INGRESS_PORT:
>> + return ROCKER_OF_DPA_TABLE_ID_INGRESS_PORT;
>> + case ROCKER_FLOW_TABLE_ID_VLAN:
>> + return ROCKER_OF_DPA_TABLE_ID_VLAN;
>> + case ROCKER_FLOW_TABLE_ID_TERMINATION_MAC:
>> + return ROCKER_OF_DPA_TABLE_ID_TERMINATION_MAC;
>> + case ROCKER_FLOW_TABLE_ID_UNICAST_ROUTING:
>> + return ROCKER_OF_DPA_TABLE_ID_UNICAST_ROUTING;
>> + case ROCKER_FLOW_TABLE_ID_MULTICAST_ROUTING:
>> + return ROCKER_OF_DPA_TABLE_ID_MULTICAST_ROUTING;
>> + case ROCKER_FLOW_TABLE_ID_BRIDGING:
>> + return ROCKER_OF_DPA_TABLE_ID_BRIDGING;
>> + case ROCKER_FLOW_TABLE_ID_ACL_POLICY:
>> + return ROCKER_OF_DPA_TABLE_ID_ACL_POLICY;
>> + default:
>> + return 0;
>> + }
>> +}
>
> Could the OF-DPA table IDs be used in the flow table defs? I think I
> remember your answer was no because OF-DPA uses INGRESS_PORT ID == 0,
> and 0 is a special value for if_flow tables. Bummer.
>
A minor nuisance. I made table_id 0 a special delineating table.
>> +
>> +static int rocker_flow_set_ig_port(struct net_device *dev,
>> + struct net_flow_flow *flow)
>> +{
>> + struct rocker_port *rocker_port = netdev_priv(dev);
>> + enum rocker_of_dpa_table_id goto_tbl;
>> + u32 in_lport_mask = 0xffff0000;
>> + u32 in_lport = 0;
>
> why initialize these two?
apparently a hold out from some code before I added the valid_net_flow()
check. I'll remove it.
>
>> + int err, flags = 0;
>> +
>> + err = is_valid_net_flow(&ingress_port_table, flow);
>> + if (err)
>> + return err;
>> +
>> + /* ingress port table only supports one field/mask/action this
>> + * simplifies the key construction and we can assume the values
>> + * are the correct types/mask/action by valid check above. The
>> + * user could pass multiple match/actions in a message with the
>> + * same field multiple times currently the valid test does not
>> + * catch this and we just use the first specified.
>> + */
>> + in_lport = flow->matches[0].value_u32;
>> + in_lport_mask = flow->matches[0].mask_u32;
>> + goto_tbl = rocker_goto_value(flow->actions[0].args[0].value_u16);
>> +
>> + err = rocker_flow_tbl_ig_port(rocker_port, flags,
>> + in_lport, in_lport_mask,
>> + goto_tbl);
>> + return err;
>> +}
>> +
>> +static int rocker_flow_set_vlan(struct net_device *dev,
>> + struct net_flow_flow *flow)
>> +{
>> + enum rocker_of_dpa_table_id goto_tbl;
>> + struct rocker_port *rocker_port = netdev_priv(dev);
>
> rocker style thing: put rocker_port decl first (sorry for being so pedantic).
no problem, making the change.
>
>> + int i, err = 0, flags = 0;
>> + u32 in_lport;
>> + __be16 vlan_id, vlan_id_mask, new_vlan_id;
>> + bool untagged, have_in_lport = false;
>> +
>> + err = is_valid_net_flow(&vlan_table, flow);
>> + if (err)
>> + return err;
>> +
>> + goto_tbl = ROCKER_OF_DPA_TABLE_ID_TERMINATION_MAC;
>> +
>> + /* If user does not specify vid match default to any */
>> + vlan_id = 1;
>
> htons()?
>
> Not sure. Rocker convention is vlan_id is network-order, but some
> places you'll see vid and that's host-order.
>
Yep this is needed.
>> + vlan_id_mask = 0;
>> +
>> + for (i = 0; flow->matches && flow->matches[i].instance; i++) {
>> + switch (flow->matches[i].instance) {
>> + case HEADER_INSTANCE_IN_LPORT:
>> + in_lport = flow->matches[i].value_u32;
>> + have_in_lport = true;
>> + break;
>> + case HEADER_INSTANCE_VLAN_OUTER:
>> + if (flow->matches[i].field != HEADER_VLAN_VID)
>> + break;
>> +
>> + vlan_id = htons(flow->matches[i].value_u16);
>> + vlan_id_mask = htons(flow->matches[i].mask_u16);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + /* If user does not specify a new vlan id use default vlan id */
>> + new_vlan_id = rocker_port_vid_to_vlan(rocker_port, vlan_id, &untagged);
>> +
>> + for (i = 0; flow->actions && flow->actions[i].uid; i++) {
>> + struct net_flow_action_arg *arg = &flow->actions[i].args[0];
>> +
>> + switch (flow->actions[i].uid) {
>> + case ACTION_SET_GOTO_TABLE:
>> + goto_tbl = rocker_goto_value(arg->value_u16);
>> + break;
>> + case ACTION_SET_VLAN_ID:
>> + new_vlan_id = htons(arg->value_u16);
>> + if (new_vlan_id)
>> + untagged = false;
>> + break;
>> + }
>> + }
>> +
>> + if (!have_in_lport)
>> + return -EINVAL;
>
> This can be moved up, before second for loop
>
done.
>> +
>> + err = rocker_flow_tbl_vlan(rocker_port, flags, in_lport,
>> + vlan_id, vlan_id_mask, goto_tbl,
>> + untagged, new_vlan_id);
>> + return err;
>> +}
>> +
>> +static int rocker_flow_set_term_mac(struct net_device *dev,
>> + struct net_flow_flow *flow)
>> +{
>> + struct rocker_port *rocker_port = netdev_priv(dev);
>> + __be16 vlan_id, vlan_id_mask, ethtype = 0;
>> + const u8 *eth_dst, *eth_dst_mask;
>> + u32 in_lport, in_lport_mask;
>> + int i, err = 0, flags = 0;
>> + bool copy_to_cpu;
>> +
>> + eth_dst = NULL;
>> + eth_dst_mask = NULL;
>> +
>
> Needed?
nope same as above hold out from an older variant of valid_net_flow().
>
>> + err = is_valid_net_flow(&term_mac_table, flow);
>> + if (err)
>> + return err;
>> +
>> + /* If user does not specify vid match default to any */
>> + vlan_id = rocker_port->internal_vlan_id;
>> + vlan_id_mask = 0;
>> +
[...]
>>
>> static const struct net_device_ops rocker_port_netdev_ops = {
>> @@ -3828,6 +4342,9 @@ static const struct net_device_ops rocker_port_netdev_ops = {
>> .ndo_flow_get_actions = rocker_get_actions,
>> .ndo_flow_get_tbl_graph = rocker_get_tgraph,
>> .ndo_flow_get_hdr_graph = rocker_get_hgraph,
>> +
>> + .ndo_flow_set_flows = rocker_set_flows,
>> + .ndo_flow_del_flows = rocker_del_flows,
>> #endif
>> };
>
> Looks good overall to me
good to hear.
>
>> diff --git a/drivers/net/ethernet/rocker/rocker_pipeline.h b/drivers/net/ethernet/rocker/rocker_pipeline.h
>> index 9544339..701e139 100644
>> --- a/drivers/net/ethernet/rocker/rocker_pipeline.h
>> +++ b/drivers/net/ethernet/rocker/rocker_pipeline.h
>> @@ -527,6 +527,7 @@ enum rocker_flow_table_id_space {
>> ROCKER_FLOW_TABLE_ID_VLAN,
>> ROCKER_FLOW_TABLE_ID_TERMINATION_MAC,
>> ROCKER_FLOW_TABLE_ID_UNICAST_ROUTING,
>> + ROCKER_FLOW_TABLE_ID_MULTICAST_ROUTING,
>> ROCKER_FLOW_TABLE_ID_BRIDGING,
>> ROCKER_FLOW_TABLE_ID_ACL_POLICY,
>> ROCKER_FLOW_TABLE_NULL = 0,
>> @@ -588,7 +589,7 @@ struct net_flow_table acl_table = {
>>
>> struct net_flow_table null_table = {
>> .name = "",
>> - .uid = 0,
>> + .uid = ROCKER_FLOW_TABLE_NULL,
>> .source = 0,
>> .size = 0,
>> .matches = NULL,
>>
>
> Move these changes to previous patch?
>
yep will do.
Thanks!
John
--
John Fastabend Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists