| 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, 10 May 2011 20:38:26 +0100
From: Ben Hutchings <bhutchings@...arflare.com>
To: Sebastian.Poehn@...den.com
Cc: netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] RFC gianfar: add rx_ntuple feature
As a general warning, you may find that the RX NFC interface makes more
sense. So far only ixgbe and sfc implement the RX n-tuple interface and
ixgbe will be moving to RX NFC.
I don't know quite what the capabilities of this hardware are, so it may
be that RX NFC doesn't make much sense.
On Tue, 2011-05-10 at 08:55 -0400, Sebastian.Poehn@...den.com wrote:
> This is the main part. Functionality to add and remove ntuples,
> conversion from ntuple to hardware binary rx filer format,
> optimization of hardware filer table entries and extended hardware
> capability check.
>
> --- gianfar_ethtool.c.orig 2011-05-10 11:45:33.301745000 +0200
> +++ gianfar_ethtool.c 2011-05-10 13:27:23.041744819 +0200
Diffs should be made from above the linux-2.6 directory (or using 'git
diff' or similar).
> @@ -42,6 +42,8 @@
>
> extern void gfar_start(struct net_device *dev);
> extern int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit);
> +extern void sort(void *, size_t, size_t, int(*cmp_func)(const void *,
> + const void *), void(*swap_func)(void *, void *, int size));
Why are you declaring this here rather than including <linux/sort.h>?
> #define GFAR_MAX_COAL_USECS 0xffff
> #define GFAR_MAX_COAL_FRAMES 0xff
> @@ -787,6 +789,1011 @@ static int gfar_set_nfc(struct net_devic
> return ret;
> }
>
> +/*Global pointer on table*/
> +struct filer_table *ref;
> +u32 filer_index;
> +struct interf *queue;
> +
> +enum nop {
> + ASC = 0, DESC = 1
> +} row;
Is this global state really necessary? I think not.
> +static inline void toggle_order(void)
> +{
> + row ^= 1;
> +}
> +
> +static int my_comp(const void *a, const void *b)
> +{
> +
> + signed int temp;
> + if (*(u32 *) a > *(u32 *) b)
> + temp = -1;
> + else if (*(u32 *) a == *(u32 *) b)
> + temp = 0;
> + else
> + temp = 1;
> +
> + if (row == DESC)
> + return temp;
> + else
> + return -temp;
> +}
Use a second comparison function to reverse the order.
> +static void my_swap(void *a, void *b, int size)
> +{
> + u32 t1 = *(u32 *) a;
> + u32 t2 = *(u32 *) (a + 4);
> + u32 t3 = *(u32 *) (a + 8);
> + u32 t4 = *(u32 *) (a + 12);
> + *(u32 *) a = *(u32 *) b;
> + *(u32 *) (a + 4) = *(u32 *) (b + 4);
> + *(u32 *) (a + 8) = *(u32 *) (b + 8);
> + *(u32 *) (a + 12) = *(u32 *) (b + 12);
> + *(u32 *) b = t1;
> + *(u32 *) (b + 4) = t2;
> + *(u32 *) (b + 8) = t3;
> + *(u32 *) (b + 12) = t4;
> +}
> +
> +/*Write a mask to hardware*/
> +static inline void set_mask(u32 mask)
> +{
> + ref->fe[filer_index].ctrl = RQFCR_AND | RQFCR_PID_MASK
> + | RQFCR_CMP_EXACT;
> + ref->fe[filer_index].prop = mask;
> + filer_index++;
> +}
> +
> +/*Sets parse bits (e.g. IP or TCP)*/
> +static void set_parse_bits(u32 host, u32 mask)
> +{
> + set_mask(mask);
> + ref->fe[filer_index].ctrl = RQFCR_CMP_EXACT | RQFCR_PID_PARSE
> + | RQFCR_AND;
> + ref->fe[filer_index].prop = host;
> + filer_index++;
> +}
> +
> +/*For setting a tuple of host,mask of type flag
> + *Example:
> + *IP-Src = 10.0.0.0/255.0.0.0
> + *host: 0x0A000000 mask: FF000000 flag: RQFPR_IPV4
> + *Note:
> + *For better usage of hardware 16 and 8 bit masks should be filled up
> + *with ones*/
> +static void set_attribute(unsigned int host, unsigned int mask,
> + unsigned int flag)
It would be clearer to rename 'host' as 'value'.
> +{
> + if (host || ~mask) {
If all bits are masked then the 'host' value must be ignored. So just
check ~mask.
> + /*This is to deal with masks smaller than 32bit
> + * and for special processing of MAC-filtering and
> + * VLAN-filtering*/
> + switch (flag) {
> + /*3bit*/
> + case RQFCR_PID_PRI:
> + if (((host & 0x7) == 0) && ((mask & 0x7) == 0))
> + return;
Doesn't this mean that an n-tuple filter that should match priority 0
will actually match all priority values?
> + host &= 0x7;
> + break;
> + /*8bit*/
> + case RQFCR_PID_L4P:
> + case RQFCR_PID_TOS:
> + if (!(mask & 0xFF))
> + mask = 0xFFFFFFFF;
I don't understand this special case. Are you sure you shouldn't be
using something like:
mask ^= 0xff;
> + break;
> + /*12bit*/
> + case RQFCR_PID_VID:
> + if (((host & 0xFFF) == 0) && ((mask & 0xFFF) == 0))
> + return;
Again, this seems to mean that a filter that should match VID 0 (i.e.
untagged) will match both tagged and untagged frames.
> + host &= 0xFFF;
> + break;
> + /*16bit*/
> + case RQFCR_PID_DPT:
> + case RQFCR_PID_SPT:
> + case RQFCR_PID_ETY:
> + if (!(mask & 0xFFFF))
> + mask = 0xFFFFFFFF;
Again, I don't understand this special case.
> + break;
> + /*24bit*/
> + case RQFCR_PID_DAH:
> + case RQFCR_PID_DAL:
> + case RQFCR_PID_SAH:
> + case RQFCR_PID_SAL:
> + host &= 0x00FFFFFF;
> + break;
> + /*for all real 32bit masks*/
> + default:
> + if (!mask)
> + mask = 0xFFFFFFFF;
> + break;
> + }
> +
> + set_mask(mask);
> + ref->fe[filer_index].ctrl = RQFCR_CMP_EXACT | RQFCR_AND | flag;
> + ref->fe[filer_index].prop = host;
> + filer_index++;
> + }
> +}
> +
> +/*Translates host and mask for UDP,TCP or SCTP*/
> +static void set_basic_ip(struct ethtool_tcpip4_spec *host,
> + struct ethtool_tcpip4_spec *mask)
> +{
> + set_attribute(host->ip4src, mask->ip4src, RQFCR_PID_SIA);
> + set_attribute(host->ip4dst, mask->ip4dst, RQFCR_PID_DIA);
> + set_attribute(host->pdst, mask->pdst | 0xFFFF0000, RQFCR_PID_DPT);
> + set_attribute(host->psrc, mask->psrc | 0xFFFF0000, RQFCR_PID_SPT);
> + set_attribute(host->tos, mask->tos | 0xFFFFFF00, RQFCR_PID_TOS);
> +}
> +
> +/*Translates host and mask for USER-IP4*/
> +static inline void set_user_ip(struct ethtool_usrip4_spec *host,
> + struct ethtool_usrip4_spec *mask)
> +{
> +
> + set_attribute(host->ip4src, mask->ip4src, RQFCR_PID_SIA);
> + set_attribute(host->ip4dst, mask->ip4dst, RQFCR_PID_DIA);
> + set_attribute(host->tos, mask->tos | 0xFFFFFF00, RQFCR_PID_TOS);
> + set_attribute(host->proto, mask->proto | 0xFFFFFF00, RQFCR_PID_L4P);
> + set_attribute(host->l4_4_bytes, mask->l4_4_bytes, RQFCR_PID_ARB);
> +
> +}
> +
> +/*Translates host and mask for ETHER spec*/
> +static inline void set_ether(struct ethhdr *host, struct ethhdr *mask)
> +{
> + u32 upper_temp_mask = 0;
> + u32 lower_temp_mask = 0;
> + /*Source address*/
> + if (!(is_zero_ether_addr(host->h_source) && is_broadcast_ether_addr(
> + mask->h_source))) {
Just check !is_broadcast_ether_addr(mask->h_source).
> + if (is_zero_ether_addr(mask->h_source)) {
> + upper_temp_mask = 0xFFFFFFFF;
> + lower_temp_mask = 0xFFFFFFFF;
> + } else {
> + upper_temp_mask = mask->h_source[0] << 16
> + | mask->h_source[1] << 8
> + | mask->h_source[2] | 0xFF000000;
> + lower_temp_mask = mask->h_source[3] << 16
> + | mask->h_source[4] << 8
> + | mask->h_source[5] | 0xFF000000;
> + }
> + /*Upper 24bit*/
> + set_attribute(0x80000000 | host->h_source[0] << 16
> + | host->h_source[1] << 8 | host->h_source[2],
> + upper_temp_mask, RQFCR_PID_SAH);
> + /*And the same for the lower part*/
> + set_attribute(0x80000000 | host->h_source[3] << 16
> + | host->h_source[4] << 8 | host->h_source[5],
> + lower_temp_mask, RQFCR_PID_SAL);
> + }
> + /*Destination address*/
> + if (!(is_zero_ether_addr(host->h_dest) && is_broadcast_ether_addr(
> + mask->h_dest))) {
Similarly here, just test the mask.
> + /*Special for destination is limited broadcast*/
> + if ((is_broadcast_ether_addr(host->h_dest)
> + && is_zero_ether_addr(mask->h_dest))) {
> + set_parse_bits(RQFPR_EBC, RQFPR_EBC);
> + } else {
> +
> + if (is_zero_ether_addr(mask->h_dest)) {
> + upper_temp_mask = 0xFFFFFFFF;
> + lower_temp_mask = 0xFFFFFFFF;
> + } else {
> + upper_temp_mask = mask->h_dest[0] << 16
> + | mask->h_dest[1] << 8
> + | mask->h_dest[2] | 0xFF000000;
> + lower_temp_mask = mask->h_dest[3] << 16
> + | mask->h_dest[4] << 8
> + | mask->h_dest[5] | 0xFF000000;
> + }
> +
> + /*Upper 24bit*/
> + set_attribute(0x80000000 | host->h_dest[0] << 16
> + | host->h_dest[1] << 8
> + | host->h_dest[2], upper_temp_mask,
> + RQFCR_PID_DAH);
> + /*And the same for the lower part*/
> + set_attribute(0x80000000 | host->h_dest[3] << 16
> + | host->h_dest[4] << 8
> + | host->h_dest[5], lower_temp_mask,
> + RQFCR_PID_DAL);
> + }
> + }
> +
> + /*Set Ethertype*/
> + if ((host->h_proto || ~(mask->h_proto | 0xFFFF0000))) {
Similarly here, just test the mask.
> + set_attribute(host->h_proto, mask->h_proto | 0xFFFF0000,
> + RQFCR_PID_ETY);
> + }
> +
> + /*
> + * Question: What the hell does the 0x80000000 do?
> + * Answer: It is just a dirty hack to prevent the setAtribute()
> + * to ignore a half MAC address which is like 0x000000/0xFFFFFF
> + */
Why would it do that?
Is a filter that matches only upper or only lower 24 bits of a MAC
address invalid?
[Skipped more stuff; I haven't got time to review all of this.]
[...]
> +static int gfar_set_rx_ntuple(struct net_device *dev,
> + struct ethtool_rx_ntuple *cmd)
> +{ struct gfar __iomem *regs = NULL;
> + struct gfar_private *priv = netdev_priv(dev);
> + int i = 0;
> + static struct interf *store[10];
> +
> + regs = priv->gfargrp[0].regs;
> +
> + /*Only values between -2 and num_rx_queues -1 allowed*/
> + if ((cmd->fs.action >= (signed int)priv->num_rx_queues) ||
> + (cmd->fs.action < ETHTOOL_RXNTUPLE_ACTION_CLEAR))
> + return -EINVAL;
> +
> + for (i = 0; i < 10; i++) {
> + if (store[i] == 0) {
> + store[i] = init_table(priv);
> + if (store[i] == (struct interf *)-1) {
> + store[i] = 0;
> + return -1;
> + }
> + strcpy(store[i]->name, dev->name);
> + break;
> + } else if (!strcmp(store[i]->name, dev->name)) {
> + queue = store[i];
> + break;
> + }
> +
> + }
Why aren't you putting this state in struct gfar_private?
You can't use name as a key anyway; interfaces can be renamed.
> + do_action(&cmd->fs, priv);
> +
> + return 0;
> +}
> +
> +
> const struct ethtool_ops gfar_ethtool_ops = {
> .get_settings = gfar_gsettings,
> .set_settings = gfar_ssettings,
> @@ -808,4 +1815,6 @@ const struct ethtool_ops gfar_ethtool_op
> .set_wol = gfar_set_wol,
> #endif
> .set_rxnfc = gfar_set_nfc,
> + /*function for accessing rx queue filer*/
> + .set_rx_ntuple = gfar_set_rx_ntuple
> };
>
> Signed-off-by: Sebastian Poehn <sebastian.poehn@...den.com>
This belongs at the top, but is not important for an RFC anyway.
> DISCLAIMER:
>
> Privileged and/or Confidential information may be contained in this
> message. If you are not the addressee of this message, you may not
> copy, use or deliver this message to anyone. In such event, you
> should destroy the message and kindly notify the sender by reply
> e-mail. It is understood that opinions or conclusions that do not
> relate to the official business of the company are neither given
> nor endorsed by the company.
Well this wasn't sent specifically to me, so am I in trouble now?
Please get rid of this nonsense.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
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