From: Or Gerlitz <ogerlitz@mellanox•com>
To: Ben Hutchings <bhutchings@solarflare•com>
Cc: <davem@davemloft•net>, <roland@kernel•org>,
<yevgenyp@mellanox•com>, <oren@mellanox•com>,
<netdev@vger•kernel.org>, Hadar Hen Zion <hadarh@mellanox•co.il>,
Amir Vadai <amirv@mellanox•co.il>
Subject: Re: [PATCH net-next 09/10] net/mlx4_en: Manage flow steering rules with ethtool
Date: Tue, 3 Jul 2012 12:00:00 +0300 [thread overview]
Message-ID: <4FF2B490.4070703@mellanox.com> (raw)
In-Reply-To: <1341158452.4852.107.camel@deadeye.wl.decadent.org.uk>
On 7/1/2012 7:00 PM, Ben Hutchings wrote:
>> +#define not_all_zeros_or_all_ones(field, type) \
>> >+ (field && (type)~field)
>> >+
>> >+static int mlx4_en_validate_flow(struct net_device *dev,
>> >+ struct ethtool_rxnfc *cmd)
>> >+{
>> >+ struct ethtool_usrip4_spec *l3_mask;
>> >+ struct ethtool_tcpip4_spec *l4_mask;
>> >+ struct ethhdr *eth_mask;
>> >+ u64 full_mac = ~0ull;
>> >+ u64 zero_mac = 0;
>> >+
>> >+ if (cmd->fs.location >= MAX_NUM_OF_FS_RULES)
>> >+ return -EINVAL;
>> >+
>> >+ switch (cmd->fs.flow_type & ~FLOW_EXT) {
>> >+ case TCP_V4_FLOW:
>> >+ case UDP_V4_FLOW:
>> >+ if (cmd->fs.h_u.tcp_ip4_spec.tos)
>> >+ return -EOPNOTSUPP;
> I suspect that your filter ignores TOS, rather than only matching TOS ==
> 0, so you should actually be checking the corresponding field in the
> mask (fs.m_u). [...]
OK, thanks for pointing this over, will fix.
> >+ break;
> >+ case IP_USER_FLOW:
> >+ l3_mask = &cmd->fs.m_u.usr_ip4_spec;
> >+ if (cmd->fs.h_u.usr_ip4_spec.l4_4_bytes ||
> >+ cmd->fs.h_u.usr_ip4_spec.tos ||
> I think this should be checking l4_4_bytes and tos in the mask.
OK
>
>> >+ cmd->fs.h_u.usr_ip4_spec.proto ||
>> >+ cmd->fs.h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
>> >+ (!cmd->fs.h_u.usr_ip4_spec.ip4src &&
>> >+ !cmd->fs.h_u.usr_ip4_spec.ip4dst) ||
>> >+ not_all_zeros_or_all_ones(l3_mask->ip4src, __be32) ||
>> >+ not_all_zeros_or_all_ones(l3_mask->ip4dst, __be32))
>> >+ return -EOPNOTSUPP;
>> >+ break;
>> >+ case ETHER_FLOW:
>> >+ eth_mask = &cmd->fs.m_u.ether_spec;
>> >+ if (memcmp(eth_mask->h_source, &zero_mac, ETH_ALEN))
>> >+ return -EOPNOTSUPP;
>> >+ if (!memcmp(eth_mask->h_dest, &zero_mac, ETH_ALEN))
>> >+ return -EOPNOTSUPP;
> But in the next statement you test whether eth_mask->h_dest is either
> all-zeroes or all-ones. Is all-zeroes valid or not? I suspect you
> actually intend to reject the case where both h_dest and h_proto are masked out.
indeed, this code section can be better written, will fix for V1
>
>> >+ if (not_all_zeros_or_all_ones(eth_mask->h_proto, __be16) ||
>> >+ (memcmp(eth_mask->h_dest, &zero_mac, ETH_ALEN) &&
>> >+ memcmp(eth_mask->h_dest, &full_mac, ETH_ALEN)))
>> >+ return -EOPNOTSUPP;
>> >+ break;
>> >+ default:
>> >+ return -EOPNOTSUPP;
>> >+ }
>> >+
>> >+ if ((cmd->fs.flow_type & FLOW_EXT)) {
>> >+ if (cmd->fs.m_ext.vlan_etype ||
>> >+ not_all_zeros_or_all_ones(cmd->fs.m_ext.vlan_tci,
>> >+ __be16)) {
>> >+ return -EOPNOTSUPP;
>> >+ }
>> >+ }
>> >+
>> >+ return 0;
>> >+}
next prev parent reply other threads:[~2012-07-03 9:00 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-01 9:43 [PATCH net-next 00/10] net/mlx4: Add flow-steering support Or Gerlitz
2012-07-01 9:43 ` [PATCH net-next 01/10] net/mlx4_core: Change resource tracking mechanism to use red-black tree Or Gerlitz
2012-07-01 10:17 ` David Miller
2012-07-01 9:43 ` [PATCH net-next 02/10] net/mlx4_core: Change resource tracking ID to be 64 bit Or Gerlitz
2012-07-01 9:43 ` [PATCH net-next 03/10] net/mlx4_en: Re-design multicast attachments flow Or Gerlitz
2012-07-01 10:32 ` David Miller
2012-07-01 9:43 ` [PATCH net-next 04/10] net/mlx4: Set steering mode according to device capabilities Or Gerlitz
2012-07-01 10:20 ` David Miller
2012-07-01 9:43 ` [PATCH net-next 05/10] net/mlx4_core: Add firmware commands to support device managed flow steering Or Gerlitz
2012-07-01 9:43 ` [PATCH net-next 06/10] {NET,IB}/mlx4: Add device managed flow steering firmware API Or Gerlitz
2012-07-01 10:30 ` David Miller
2012-07-01 12:29 ` Or Gerlitz
2012-07-01 21:42 ` David Miller
2012-07-02 7:55 ` Or Gerlitz
2012-07-02 8:14 ` Roland Dreier
2012-07-02 8:28 ` Or Gerlitz
2012-07-02 8:36 ` David Miller
2012-07-02 13:00 ` Or Gerlitz
2012-07-02 8:34 ` David Miller
2012-07-02 18:07 ` Ben Hutchings
2012-07-03 0:15 ` David Miller
2012-07-03 1:04 ` David Miller
2012-07-03 11:10 ` Or Gerlitz
2012-07-01 9:43 ` [PATCH net-next 07/10] net/mlx4_core: Add resource tracking for device managed flow steering rules Or Gerlitz
2012-07-01 10:22 ` David Miller
2012-07-01 9:43 ` [PATCH net-next 08/10] net/mlx4: Implement promiscuous mode with device managed flow-steering Or Gerlitz
2012-07-01 9:43 ` [PATCH net-next 09/10] net/mlx4_en: Manage flow steering rules with ethtool Or Gerlitz
2012-07-01 10:23 ` David Miller
2012-07-01 16:00 ` Ben Hutchings
2012-07-01 16:38 ` Joe Perches
2012-07-01 17:31 ` Joe Perches
2012-07-01 18:48 ` Andreas Schwab
2012-07-01 19:52 ` Joe Perches
2012-07-02 10:19 ` David Laight
2012-07-02 11:35 ` Andreas Schwab
2012-07-02 12:15 ` David Laight
2012-07-03 8:14 ` Or Gerlitz
2012-07-02 12:57 ` Or Gerlitz
2012-07-03 1:47 ` Ben Hutchings
2012-07-03 8:56 ` Or Gerlitz
2012-07-03 8:58 ` Or Gerlitz
2012-07-02 13:32 ` Or Gerlitz
2012-07-03 1:50 ` Ben Hutchings
2012-07-03 9:00 ` Or Gerlitz [this message]
2012-07-01 9:43 ` [PATCH net-next 10/10] net/mlx4_en: Add support for drop action through ethtool Or Gerlitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FF2B490.4070703@mellanox.com \
--to=ogerlitz@mellanox$(echo .)com \
--cc=amirv@mellanox$(echo .)co.il \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=hadarh@mellanox$(echo .)co.il \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oren@mellanox$(echo .)com \
--cc=roland@kernel$(echo .)org \
--cc=yevgenyp@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox