* [PATCH v2 11/15] stm class: make config_item_type const
From: Bhumika Goyal @ 2017-10-16 15:18 UTC (permalink / raw)
To: julia.lawall, rjw, lenb, alexander.shishkin, jic23, knaack.h,
lars, pmeerw, dledford, sean.hefty, hal.rosenstock, hch, sagi,
kishon, bhelgaas, nab, balbi, gregkh, laurent.pinchart, jlbec,
ccaulfie, teigland, mfasheh, linux-acpi, linux-kernel, linux-iio,
linux-rdma, netdev, linux-nvme, linux-pci, linux-scsi,
target-devel, linux-usb, cluster-devel
Cc: Bhumika Goyal
In-Reply-To: <1508167134-6243-1-git-send-email-bhumirks@gmail.com>
Make config_item_type structures const as they are either passed to a
function having the argument as const or used inside a if statement or
stored in the const "ci_type" field of a config_item structure.
Done using Coccinelle.
Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
* Changes in v2- Combine all the followup patches and the constification
patches into a series.
drivers/hwtracing/stm/policy.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 6c0ae29..33e9a1b 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -187,8 +187,8 @@ static void stp_policy_node_release(struct config_item *item)
NULL,
};
-static struct config_item_type stp_policy_type;
-static struct config_item_type stp_policy_node_type;
+static const struct config_item_type stp_policy_type;
+static const struct config_item_type stp_policy_node_type;
static struct config_group *
stp_policy_node_make(struct config_group *group, const char *name)
@@ -236,7 +236,7 @@ static void stp_policy_node_release(struct config_item *item)
.drop_item = stp_policy_node_drop,
};
-static struct config_item_type stp_policy_node_type = {
+static const struct config_item_type stp_policy_node_type = {
.ct_item_ops = &stp_policy_node_item_ops,
.ct_group_ops = &stp_policy_node_group_ops,
.ct_attrs = stp_policy_node_attrs,
@@ -311,7 +311,7 @@ static void stp_policy_release(struct config_item *item)
.make_group = stp_policy_node_make,
};
-static struct config_item_type stp_policy_type = {
+static const struct config_item_type stp_policy_type = {
.ct_item_ops = &stp_policy_item_ops,
.ct_group_ops = &stp_policy_group_ops,
.ct_attrs = stp_policy_attrs,
@@ -380,7 +380,7 @@ static void stp_policy_release(struct config_item *item)
.make_group = stp_policies_make,
};
-static struct config_item_type stp_policies_type = {
+static const struct config_item_type stp_policies_type = {
.ct_group_ops = &stp_policies_group_ops,
.ct_owner = THIS_MODULE,
};
--
1.9.1
^ permalink raw reply related
* [PATCH v2 15/15] configfs: make config_item_type const
From: Bhumika Goyal @ 2017-10-16 15:18 UTC (permalink / raw)
To: julia.lawall, rjw, lenb, alexander.shishkin, jic23, knaack.h,
lars, pmeerw, dledford, sean.hefty, hal.rosenstock, hch, sagi,
kishon, bhelgaas, nab, balbi, gregkh, laurent.pinchart, jlbec,
ccaulfie, teigland, mfasheh, linux-acpi, linux-kernel, linux-iio,
linux-rdma, netdev, linux-nvme, linux-pci, linux-scsi,
target-devel, linux-usb, cluster-devel
Cc: Bhumika Goyal
In-Reply-To: <1508167134-6243-1-git-send-email-bhumirks@gmail.com>
Make config_item_type structures const as they are either passed to a
function having the argument as const or stored in the const "ci_type"
field of a config_item structure.
Done using Coccinelle.
Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
* Changes in v2- Combine all the followup patches and the constification
patches into a series.
samples/configfs/configfs_sample.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/samples/configfs/configfs_sample.c b/samples/configfs/configfs_sample.c
index 1ea3311..004a4e2 100644
--- a/samples/configfs/configfs_sample.c
+++ b/samples/configfs/configfs_sample.c
@@ -115,7 +115,7 @@ static ssize_t childless_description_show(struct config_item *item, char *page)
NULL,
};
-static struct config_item_type childless_type = {
+static const struct config_item_type childless_type = {
.ct_attrs = childless_attrs,
.ct_owner = THIS_MODULE,
};
@@ -193,7 +193,7 @@ static void simple_child_release(struct config_item *item)
.release = simple_child_release,
};
-static struct config_item_type simple_child_type = {
+static const struct config_item_type simple_child_type = {
.ct_item_ops = &simple_child_item_ops,
.ct_attrs = simple_child_attrs,
.ct_owner = THIS_MODULE,
@@ -261,7 +261,7 @@ static void simple_children_release(struct config_item *item)
.make_item = simple_children_make_item,
};
-static struct config_item_type simple_children_type = {
+static const struct config_item_type simple_children_type = {
.ct_item_ops = &simple_children_item_ops,
.ct_group_ops = &simple_children_group_ops,
.ct_attrs = simple_children_attrs,
@@ -331,7 +331,7 @@ static ssize_t group_children_description_show(struct config_item *item,
.make_group = group_children_make_group,
};
-static struct config_item_type group_children_type = {
+static const struct config_item_type group_children_type = {
.ct_group_ops = &group_children_group_ops,
.ct_attrs = group_children_attrs,
.ct_owner = THIS_MODULE,
--
1.9.1
^ permalink raw reply related
* RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
From: Rodney Cummings @ 2017-10-16 15:23 UTC (permalink / raw)
To: Vivien Didelot, netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,
David S. Miller, Florian Fainelli, Andrew Lunn, David Laight
In-Reply-To: <20171013181809.14627-1-vivien.didelot@savoirfairelinux.com>
I am concerned about this proposed change.
According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end station) that is internal to the bridge (i.e. switch) can use the same individual global MAC address that the switch is using. That behavior is common.
Use of a local semi-random MAC address is dangerous on Ethernet. Local MAC addresses can only be safely used in very narrow use cases, most of which are Wi-Fi. Generally speaking, local MAC addresses can be duplicated in the network, which causes many Ethernet protocols to break down. Therefore, general-purpose implementations like DSA cannot use a local MAC address in a reliable manner.
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Vivien Didelot
> Sent: Friday, October 13, 2017 1:18 PM
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; kernel@savoirfairelinux.com; David S.
> Miller <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>;
> Andrew Lunn <andrew@lunn.ch>; David Laight <David.Laight@ACULAB.COM>;
> Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Subject: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
>
> An Ethernet switch may support having a MAC address, which can be used
> as the switch's source address in transmitted full-duplex Pause frames.
>
> If a DSA switch supports the related .set_addr operation, the DSA core
> sets the master's MAC address on the switch.
>
> This won't make sense anymore in a multi-CPU ports system, because there
> won't be a unique master device assigned to a switch tree.
>
> Moreover this operation is confusing because it makes the user think
> that it could be used to program the switch with the MAC address of the
> CPU/management port such that MAC address learning can be disabled on
> said port, but in fact, that's not how it is currently used.
>
> To fix this, assign a random MAC address at setup time in the mv88e6060
> and mv88e6xxx drivers before removing .set_addr completely from DSA.
>
> Changes in v3:
> - include fix for mv88e6060 switch MAC address setter.
>
> Changes in v2:
> - remove .set_addr implementation from drivers and use a random MAC.
>
> Vivien Didelot (5):
> net: dsa: mv88e6xxx: setup random mac address
> net: dsa: mv88e6060: fix switch MAC address
> net: dsa: mv88e6060: setup random mac address
> net: dsa: dsa_loop: remove .set_addr
> net: dsa: remove .set_addr
>
> drivers/net/dsa/dsa_loop.c | 8 --------
> drivers/net/dsa/mv88e6060.c | 37 ++++++++++++++++++++++++++---------
> --
> drivers/net/dsa/mv88e6xxx/chip.c | 33 +++++++++++++++++----------------
> include/net/dsa.h | 1 -
> net/dsa/dsa2.c | 6 ------
> net/dsa/legacy.c | 6 ------
> 6 files changed, 43 insertions(+), 48 deletions(-)
>
> --
> 2.14.2
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-10-16 15:31 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Network Development, David Miller, Jason Wang, virtualization,
Willem de Bruijn
In-Reply-To: <CAF=yD-LdGiKuf=k2FDgFWbhvExVX8t+VVgTQetKTegjMeQ598g@mail.gmail.com>
On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
> >> +static int virtnet_reset(struct virtnet_info *vi)
> >> +{
> >> + struct virtio_device *dev = vi->vdev;
> >> + int ret;
> >> +
> >> + virtio_config_disable(dev);
> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> >> + virtnet_freeze_down(dev, true);
> >> + remove_vq_common(vi);
> >> +
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >> +
> >> + ret = virtio_finalize_features(dev);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = virtnet_restore_up(dev);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> + virtio_config_enable(dev);
> >> + return 0;
> >> +
> >> +err:
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> >> + return ret;
> >> +}
> >> +
> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> >> {
> >> struct scatterlist sg;
> >
> > I have a question here though. How do things like MAC address
> > get restored?
> >
> > What about the rx mode?
> >
> > vlans?
>
> The function as is releases and reinitializes only ring state.
> Device configuration such as mac and vlan persist across
> the reset.
What gave you this impression? Take a look at e.g. this
code in qemu:
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
/* Reset back to compatibility mode */
n->promisc = 1;
n->allmulti = 0;
n->alluni = 0;
n->nomulti = 0;
n->nouni = 0;
n->nobcast = 0;
/* multiqueue is disabled by default */
n->curr_queues = 1;
timer_del(n->announce_timer);
n->announce_counter = 0;
n->status &= ~VIRTIO_NET_S_ANNOUNCE;
/* Flush any MAC and VLAN filter table state */
n->mac_table.in_use = 0;
n->mac_table.first_multi = 0;
n->mac_table.multi_overflow = 0;
n->mac_table.uni_overflow = 0;
memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
memset(n->vlans, 0, MAX_VLAN >> 3);
}
So device seems to lose all state, you have to re-program it.
> > Also, it seems that LINK_ANNOUNCE requests will get ignored
> > even if they got set before the reset, leading to downtime.
>
> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
> requests? That flag is tested and netdev_notify_peers
> called before resetting virtio ring state.
Yes but I wonder if there's a race where announce
is set after it is read but before NEED_RESET is read.
Re-reading status from the config before reset
might be necessary.
--
MST
^ permalink raw reply
* [PATCH net-next 0/2] TCP experimental option for SMC rendezvous
From: Ursula Braun @ 2017-10-16 15:42 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390, jwi, schwidefsky, heiko.carstens, raspl,
ubraun
Dave,
SMC-capability is to be negotiated with a TCP experimental option.
As requested during code review of our previous approach using
netfilter hooks, here's a new version. It touches tcp-code in the
first patch and exploits the new tcp flag in the smc-code.
Kind regards, Ursula
Ursula Braun (2):
tcp: TCP experimental option for SMC
smc: add SMC rendezvous protocol
include/linux/tcp.h | 9 ++++--
include/net/inet_sock.h | 3 +-
include/net/tcp.h | 8 ++++++
net/ipv4/tcp.c | 6 ++++
net/ipv4/tcp_input.c | 45 ++++++++++++++++++++++++++++++
net/ipv4/tcp_minisocks.c | 20 ++++++++++++++
net/ipv4/tcp_output.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++--
net/smc/af_smc.c | 21 +++++++++++++-
8 files changed, 176 insertions(+), 7 deletions(-)
--
2.13.5
^ permalink raw reply
* [PATCH net-next 2/2] smc: add SMC rendezvous protocol
From: Ursula Braun @ 2017-10-16 15:42 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390, jwi, schwidefsky, heiko.carstens, raspl,
ubraun
In-Reply-To: <20171016154202.72635-1-ubraun@linux.vnet.ibm.com>
The SMC protocol [1] uses a rendezvous protocol to negotiate SMC
capability between peers. The current Linux implementation does not yet
use this rendezvous protocol and, thus, is not compliant to RFC7609 and
incompatible with other SMC implementations like in zOS.
This patch adds support for the SMC rendezvous protocol. It uses a new
TCP experimental option. With this option, SMC capabilities are
exchanged between the peers during the TCP three way handshake.
[1] SMC-R Informational RFC: http://www.rfc-editor.org/info/rfc7609
Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
---
net/smc/af_smc.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 745f145d4c4d..f1842da9d90e 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -390,6 +390,12 @@ static int smc_connect_rdma(struct smc_sock *smc)
int rc = 0;
u8 ibport;
+ if (!tcp_sk(smc->clcsock->sk)->syn_smc) {
+ /* peer has not signalled SMC-capability */
+ smc->use_fallback = true;
+ goto out_connected;
+ }
+
/* IPSec connections opt out of SMC-R optimizations */
if (using_ipsec(smc)) {
reason_code = SMC_CLC_DECL_IPSEC;
@@ -555,6 +561,7 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
}
smc_copy_sock_settings_to_clc(smc);
+ tcp_sk(smc->clcsock->sk)->syn_smc = 1;
rc = kernel_connect(smc->clcsock, addr, alen, flags);
if (rc)
goto out;
@@ -752,6 +759,7 @@ static void smc_listen_work(struct work_struct *work)
struct smc_clc_msg_proposal pclc;
struct smc_ib_device *smcibdev;
struct sockaddr_in peeraddr;
+ bool lgr_lock_taken = false;
struct smc_link *link;
int reason_code = 0;
int rc = 0, len;
@@ -759,6 +767,12 @@ static void smc_listen_work(struct work_struct *work)
u8 prefix_len;
u8 ibport;
+ /* check if peer is smc capable */
+ if (!tcp_sk(newclcsock->sk)->syn_smc) {
+ new_smc->use_fallback = true;
+ goto out_connected;
+ }
+
/* do inband token exchange -
*wait for and receive SMC Proposal CLC message
*/
@@ -802,6 +816,7 @@ static void smc_listen_work(struct work_struct *work)
/* allocate connection / link group */
mutex_lock(&smc_create_lgr_pending);
+ lgr_lock_taken = true;
local_contact = smc_conn_create(new_smc, peeraddr.sin_addr.s_addr,
smcibdev, ibport, &pclc.lcl, 0);
if (local_contact < 0) {
@@ -882,7 +897,8 @@ static void smc_listen_work(struct work_struct *work)
if (newsmcsk->sk_state == SMC_INIT)
newsmcsk->sk_state = SMC_ACTIVE;
enqueue:
- mutex_unlock(&smc_create_lgr_pending);
+ if (lgr_lock_taken)
+ mutex_unlock(&smc_create_lgr_pending);
lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
if (lsmc->sk.sk_state == SMC_LISTEN) {
smc_accept_enqueue(&lsmc->sk, newsmcsk);
@@ -963,6 +979,7 @@ static int smc_listen(struct socket *sock, int backlog)
* them to the clc socket -- copy smc socket options to clc socket
*/
smc_copy_sock_settings_to_clc(smc);
+ tcp_sk(smc->clcsock->sk)->syn_smc = 1;
rc = kernel_listen(smc->clcsock, backlog);
if (rc)
@@ -1405,6 +1422,7 @@ static int __init smc_init(void)
goto out_sock;
}
+ static_key_slow_inc(&tcp_have_smc);
return 0;
out_sock:
@@ -1429,6 +1447,7 @@ static void __exit smc_exit(void)
list_del_init(&lgr->list);
smc_lgr_free(lgr); /* free link group */
}
+ static_key_slow_dec(&tcp_have_smc);
smc_ib_unregister_client();
sock_unregister(PF_SMC);
proto_unregister(&smc_proto);
--
2.13.5
^ permalink raw reply related
* [PATCH net-next 1/2] tcp: TCP experimental option for SMC
From: Ursula Braun @ 2017-10-16 15:42 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-s390, jwi, schwidefsky, heiko.carstens, raspl,
ubraun
In-Reply-To: <20171016154202.72635-1-ubraun@linux.vnet.ibm.com>
The SMC protocol [1] relies on the use of a new TCP experimental
option [2, 3]. With this option, SMC capabilities are exchanged
between peers during the TCP three way handshake. This patch adds
support for this experimental option to TCP.
References:
[1] SMC-R Informational RFC: http://www.rfc-editor.org/info/rfc7609
[2] Shared Use of TCP Experimental Options RFC 6994:
https://tools.ietf.org/rfc/rfc6994.txt
[3] IANA ExID SMCR:
http://www.iana.org/assignments/tcp-parameters/tcp-parameters.xhtml#tcp-exids
Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
---
include/linux/tcp.h | 9 ++++--
include/net/inet_sock.h | 3 +-
include/net/tcp.h | 8 ++++++
net/ipv4/tcp.c | 6 ++++
net/ipv4/tcp_input.c | 45 ++++++++++++++++++++++++++++++
net/ipv4/tcp_minisocks.c | 20 ++++++++++++++
net/ipv4/tcp_output.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++--
7 files changed, 156 insertions(+), 6 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 1d2c44e09e31..3fb28954a1b7 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -98,7 +98,8 @@ struct tcp_options_received {
tstamp_ok : 1, /* TIMESTAMP seen on SYN packet */
dsack : 1, /* D-SACK is scheduled */
wscale_ok : 1, /* Wscale seen on SYN packet */
- sack_ok : 4, /* SACK seen on SYN packet */
+ sack_ok : 3, /* SACK seen on SYN packet */
+ smc_ok : 1, /* SMC seen on SYN packet */
snd_wscale : 4, /* Window scaling received from sender */
rcv_wscale : 4; /* Window scaling to send to receiver */
u8 num_sacks; /* Number of SACK blocks */
@@ -110,6 +111,9 @@ static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
{
rx_opt->tstamp_ok = rx_opt->sack_ok = 0;
rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+#if IS_ENABLED(CONFIG_SMC)
+ rx_opt->smc_ok = 0;
+#endif
}
/* This is the max number of SACKS that we'll generate and process. It's safe
@@ -228,7 +232,8 @@ struct tcp_sock {
syn_fastopen_ch:1, /* Active TFO re-enabling probe */
syn_data_acked:1,/* data in SYN is acked by SYN-ACK */
save_syn:1, /* Save headers of SYN packet */
- is_cwnd_limited:1;/* forward progress limited by snd_cwnd? */
+ is_cwnd_limited:1,/* forward progress limited by snd_cwnd? */
+ syn_smc:1; /* SYN includes SMC */
u32 tlp_high_seq; /* snd_nxt at the time of TLP retransmit. */
/* RTT measurement */
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index aa95053dfc78..600a7626eba0 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -92,7 +92,8 @@ struct inet_request_sock {
wscale_ok : 1,
ecn_ok : 1,
acked : 1,
- no_srccheck: 1;
+ no_srccheck: 1,
+ smc_ok : 1;
kmemcheck_bitfield_end(flags);
u32 ir_mark;
union {
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3b3b9b968e2d..d8a82a35c1b2 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -45,6 +45,7 @@
#include <linux/seq_file.h>
#include <linux/memcontrol.h>
+#include <linux/unaligned/access_ok.h>
#include <linux/bpf.h>
#include <linux/filter.h>
@@ -191,6 +192,7 @@ void tcp_time_wait(struct sock *sk, int state, int timeo);
* experimental options. See draft-ietf-tcpm-experimental-options-00.txt
*/
#define TCPOPT_FASTOPEN_MAGIC 0xF989
+#define TCPOPT_SMC_MAGIC 0xE2D4C3D9
/*
* TCP option lengths
@@ -203,6 +205,7 @@ void tcp_time_wait(struct sock *sk, int state, int timeo);
#define TCPOLEN_MD5SIG 18
#define TCPOLEN_FASTOPEN_BASE 2
#define TCPOLEN_EXP_FASTOPEN_BASE 4
+#define TCPOLEN_EXP_SMC_BASE 6
/* But this is what stacks really send out. */
#define TCPOLEN_TSTAMP_ALIGNED 12
@@ -213,6 +216,7 @@ void tcp_time_wait(struct sock *sk, int state, int timeo);
#define TCPOLEN_SACK_PERBLOCK 8
#define TCPOLEN_MD5SIG_ALIGNED 20
#define TCPOLEN_MSS_ALIGNED 4
+#define TCPOLEN_EXP_SMC_BASE_ALIGNED 8
/* Flags in tp->nonagle */
#define TCP_NAGLE_OFF 1 /* Nagle's algo is disabled */
@@ -2101,4 +2105,8 @@ static inline bool tcp_bpf_ca_needs_ecn(struct sock *sk)
{
return (tcp_call_bpf(sk, BPF_SOCK_OPS_NEEDS_ECN) == 1);
}
+
+#if IS_ENABLED(CONFIG_SMC)
+extern struct static_key tcp_have_smc;
+#endif
#endif /* _TCP_H */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3b34850d361f..d2d7e1ad0897 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -270,6 +270,7 @@
#include <linux/time.h>
#include <linux/slab.h>
#include <linux/errqueue.h>
+#include <linux/static_key.h>
#include <net/icmp.h>
#include <net/inet_common.h>
@@ -300,6 +301,11 @@ EXPORT_SYMBOL(sysctl_tcp_wmem);
atomic_long_t tcp_memory_allocated; /* Current allocated memory. */
EXPORT_SYMBOL(tcp_memory_allocated);
+#if IS_ENABLED(CONFIG_SMC)
+struct static_key tcp_have_smc __read_mostly = STATIC_KEY_INIT_FALSE;
+EXPORT_SYMBOL(tcp_have_smc);
+#endif
+
/*
* Current number of TCP sockets.
*/
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d0682ce2a5d6..28d03af2c7b1 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -75,6 +75,7 @@
#include <linux/ipsec.h>
#include <asm/unaligned.h>
#include <linux/errqueue.h>
+#include <linux/static_key.h>
int sysctl_tcp_fack __read_mostly;
int sysctl_tcp_max_reordering __read_mostly = 300;
@@ -3735,6 +3736,21 @@ static void tcp_parse_fastopen_option(int len, const unsigned char *cookie,
foc->exp = exp_opt;
}
+static void smc_parse_options(const struct tcphdr *th,
+ struct tcp_options_received *opt_rx,
+ const unsigned char *ptr,
+ int opsize)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ if (!static_key_false(&tcp_have_smc))
+ return;
+ if (th->syn && !(opsize & 1) &&
+ opsize >= TCPOLEN_EXP_SMC_BASE &&
+ get_unaligned_be32(ptr) == TCPOPT_SMC_MAGIC)
+ opt_rx->smc_ok = 1;
+#endif
+}
+
/* Look for tcp options. Normally only called on SYN and SYNACK packets.
* But, this can also be called on packets in the established flow when
* the fast version below fails.
@@ -3842,6 +3858,9 @@ void tcp_parse_options(const struct net *net,
tcp_parse_fastopen_option(opsize -
TCPOLEN_EXP_FASTOPEN_BASE,
ptr + 2, th->syn, foc, true);
+ else
+ smc_parse_options(th, opt_rx, ptr,
+ opsize);
break;
}
@@ -5594,6 +5613,15 @@ static bool tcp_rcv_fastopen_synack(struct sock *sk, struct sk_buff *synack,
return false;
}
+static void smc_check_reset_syn(struct tcp_sock *tp)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ if (static_key_false(&tcp_have_smc))
+ if (tp->syn_smc && !tp->rx_opt.smc_ok)
+ tp->syn_smc = 0;
+#endif
+}
+
static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
const struct tcphdr *th)
{
@@ -5700,6 +5728,8 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
* is initialized. */
tp->copied_seq = tp->rcv_nxt;
+ smc_check_reset_syn(tp);
+
smp_mb();
tcp_finish_connect(sk, skb);
@@ -6129,6 +6159,20 @@ static void tcp_ecn_create_request(struct request_sock *req,
inet_rsk(req)->ecn_ok = 1;
}
+static void smc_set_capability(struct inet_request_sock *ireq,
+ const struct tcp_options_received *rx_opt)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ if (!static_key_false(&tcp_have_smc))
+ return;
+
+ if (rx_opt->smc_ok)
+ ireq->smc_ok = 1;
+ else
+ ireq->smc_ok = 0;
+#endif
+}
+
static void tcp_openreq_init(struct request_sock *req,
const struct tcp_options_received *rx_opt,
struct sk_buff *skb, const struct sock *sk)
@@ -6152,6 +6196,7 @@ static void tcp_openreq_init(struct request_sock *req,
ireq->ir_rmt_port = tcp_hdr(skb)->source;
ireq->ir_num = ntohs(tcp_hdr(skb)->dest);
ireq->ir_mark = inet_request_mark(sk, skb);
+ smc_set_capability(ireq, rx_opt);
}
struct request_sock *inet_reqsk_alloc(const struct request_sock_ops *ops,
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 2341b9f857b6..2b1bff09a8c3 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <linux/sysctl.h>
#include <linux/workqueue.h>
+#include <linux/static_key.h>
#include <net/tcp.h>
#include <net/inet_common.h>
#include <net/xfrm.h>
@@ -417,6 +418,22 @@ void tcp_ca_openreq_child(struct sock *sk, const struct dst_entry *dst)
}
EXPORT_SYMBOL_GPL(tcp_ca_openreq_child);
+static void smc_check_reset_syn_req(struct tcp_sock *oldtp,
+ struct request_sock *req,
+ struct tcp_sock *newtp)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ struct inet_request_sock *ireq;
+
+ if (!static_key_false(&tcp_have_smc))
+ return;
+
+ ireq = inet_rsk(req);
+ if (oldtp->syn_smc && !ireq->smc_ok)
+ newtp->syn_smc = 0;
+#endif
+}
+
/* This is not only more efficient than what we used to do, it eliminates
* a lot of code duplication between IPv4/IPv6 SYN recv processing. -DaveM
*
@@ -434,6 +451,9 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
struct tcp_request_sock *treq = tcp_rsk(req);
struct inet_connection_sock *newicsk = inet_csk(newsk);
struct tcp_sock *newtp = tcp_sk(newsk);
+ struct tcp_sock *oldtp = tcp_sk(sk);
+
+ smc_check_reset_syn_req(oldtp, req, newtp);
/* Now setup tcp_sock */
newtp->pred_flags = 0;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 6c74f2a39778..36b5e45f02ca 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -41,6 +41,7 @@
#include <linux/compiler.h>
#include <linux/gfp.h>
#include <linux/module.h>
+#include <linux/static_key.h>
#include <trace/events/tcp.h>
@@ -422,6 +423,22 @@ static inline bool tcp_urg_mode(const struct tcp_sock *tp)
#define OPTION_MD5 (1 << 2)
#define OPTION_WSCALE (1 << 3)
#define OPTION_FAST_OPEN_COOKIE (1 << 8)
+#define OPTION_SMC (1 << 9)
+
+static void smc_options_write(__be32 *ptr, u16 *options)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ if (!static_key_false(&tcp_have_smc))
+ return;
+ if (unlikely(OPTION_SMC & *options)) {
+ *ptr++ = htonl((TCPOPT_NOP << 24) |
+ (TCPOPT_NOP << 16) |
+ (TCPOPT_EXP << 8) |
+ (TCPOLEN_EXP_SMC_BASE));
+ *ptr++ = htonl(TCPOPT_SMC_MAGIC);
+ }
+#endif
+}
struct tcp_out_options {
u16 options; /* bit field of OPTION_* */
@@ -540,6 +557,49 @@ static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
}
ptr += (len + 3) >> 2;
}
+
+ smc_options_write(ptr, &options);
+}
+
+static void smc_set_option(const struct tcp_sock *tp,
+ struct tcp_out_options *opts,
+ unsigned int *remaining)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ if (!static_key_false(&tcp_have_smc))
+ return;
+ if (tp->syn_smc) {
+ u32 need = TCPOLEN_EXP_SMC_BASE_ALIGNED;
+
+ if (*remaining >= need) {
+ opts->options |= OPTION_SMC;
+ *remaining -= need;
+ }
+ }
+#endif
+}
+
+static void smc_set_option_cond(const struct tcp_sock *tp,
+ const struct request_sock *req,
+ struct tcp_out_options *opts,
+ unsigned int *remaining)
+{
+#if IS_ENABLED(CONFIG_SMC)
+ struct inet_request_sock *ireq;
+
+ if (!static_key_false(&tcp_have_smc))
+ return;
+
+ ireq = inet_rsk(req);
+ if (tp->syn_smc && ireq->smc_ok) {
+ u32 need = TCPOLEN_EXP_SMC_BASE_ALIGNED;
+
+ if (*remaining >= need) {
+ opts->options |= OPTION_SMC;
+ *remaining -= need;
+ }
+ }
+#endif
}
/* Compute TCP options for SYN packets. This is not the final
@@ -607,11 +667,14 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
}
}
+ smc_set_option(tp, opts, &remaining);
+
return MAX_TCP_OPTION_SPACE - remaining;
}
/* Set up TCP options for SYN-ACKs. */
-static unsigned int tcp_synack_options(struct request_sock *req,
+static unsigned int tcp_synack_options(const struct sock *sk,
+ struct request_sock *req,
unsigned int mss, struct sk_buff *skb,
struct tcp_out_options *opts,
const struct tcp_md5sig_key *md5,
@@ -667,6 +730,8 @@ static unsigned int tcp_synack_options(struct request_sock *req,
}
}
+ smc_set_option_cond(tcp_sk(sk), req, opts, &remaining);
+
return MAX_TCP_OPTION_SPACE - remaining;
}
@@ -3193,8 +3258,8 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
md5 = tcp_rsk(req)->af_specific->req_md5_lookup(sk, req_to_sk(req));
#endif
skb_set_hash(skb, tcp_rsk(req)->txhash, PKT_HASH_TYPE_L4);
- tcp_header_size = tcp_synack_options(req, mss, skb, &opts, md5, foc) +
- sizeof(*th);
+ tcp_header_size = tcp_synack_options(sk, req, mss, skb, &opts, md5,
+ foc) + sizeof(*th);
skb_push(skb, tcp_header_size);
skb_reset_transport_header(skb);
--
2.13.5
^ permalink raw reply related
* [PATCH net] bpf: disallow arithmetic operations on context pointer
From: Jakub Kicinski @ 2017-10-16 15:45 UTC (permalink / raw)
To: netdev; +Cc: oss-drivers, alexei.starovoitov, daniel, ecree, Jakub Kicinski
Commit f1174f77b50c ("bpf/verifier: rework value tracking")
removed the crafty selection of which pointer types are
allowed to be modified. This is OK for most pointer types
since adjust_ptr_min_max_vals() will catch operations on
immutable pointers. One exception is PTR_TO_CTX which is
now allowed to be offseted freely.
The intent of aforementioned commit was to allow context
access via modified registers. The offset passed to
->is_valid_access() verifier callback has been adjusted
by the value of the variable offset.
What is missing, however, is taking the variable offset
into account when the context register is used. Or in terms
of the code adding the offset to the value passed to the
->convert_ctx_access() callback. This leads to the following
eBPF user code:
r1 += 68
r0 = *(u32 *)(r1 + 8)
exit
being translated to this in kernel space:
0: (07) r1 += 68
1: (61) r0 = *(u32 *)(r1 +180)
2: (95) exit
Offset 8 is corresponding to 180 in the kernel, but offset
76 is valid too. Verifier will "accept" access to offset
68+8=76 but then "convert" access to offset 8 as 180.
Effective access to offset 248 is beyond the kernel context.
(This is a __sk_buff example on a debug-heavy kernel -
packet mark is 8 -> 180, 76 would be data.)
Dereferencing the modified context pointer is not as easy
as dereferencing other types, because we have to translate
the access to reading a field in kernel structures which is
usually at a different offset and often of a different size.
To allow modifying the pointer we would have to make sure
that given eBPF instruction will always access the same
field or the fields accessed are "compatible" in terms of
offset and size...
Disallow dereferencing modified context pointers and add
to selftests the test case described here.
Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
Dave, a merge note - in net-next this will need env to be passed
to verbose().
kernel/bpf/verifier.c | 8 ++++++--
tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8b8d6ba39e23..8499759d0c7a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
/* ctx accesses must be at a fixed offset, so that we can
* determine what type of data were returned.
*/
- if (!tnum_is_const(reg->var_off)) {
+ if (reg->off) {
+ verbose("derefence of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
+ regno, reg->off, off - reg->off);
+ return -EACCES;
+ }
+ if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
char tn_buf[48];
tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
@@ -1124,7 +1129,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
tn_buf, off, size);
return -EACCES;
}
- off += reg->var_off.value;
err = check_ctx_access(env, insn_idx, off, size, t, ®_type);
if (!err && t == BPF_READ && value_regno >= 0) {
/* ctx access returns either a scalar, or a
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 26f3250bdcd2..d41c77e7b39b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -6645,6 +6645,20 @@ static struct bpf_test tests[] = {
.errstr = "BPF_END uses reserved fields",
.result = REJECT,
},
+ {
+ "arithmetic ops make PTR_TO_CTX unusable",
+ .insns = {
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_1,
+ offsetof(struct __sk_buff, data) -
+ offsetof(struct __sk_buff, mark)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
+ offsetof(struct __sk_buff, mark)),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "derefence of modified ctx ptr R1 off=68+8, ctx+const is allowed, ctx+const+const is not",
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
--
2.14.1
^ permalink raw reply related
* Re: [net-next 4/9] i40e: Enable 'channel' mode in mqprio for TC configs
From: Nambiar, Amritha @ 2017-10-16 16:03 UTC (permalink / raw)
To: Yunsheng Lin, Jeff Kirsher, davem; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <7ba989ca-ab23-1572-f125-a851f87ad05a@huawei.com>
On 10/16/2017 1:53 AM, Yunsheng Lin wrote:
> Hi, Jeff
>
> On 2017/10/14 5:52, Jeff Kirsher wrote:
>> From: Amritha Nambiar <amritha.nambiar@intel.com>
>>
>> The i40e driver is modified to enable the new mqprio hardware
>> offload mode and factor the TCs and queue configuration by
>> creating channel VSIs. In this mode, the priority to traffic
>> class mapping and the user specified queue ranges are used
>> to configure the traffic classes by setting the mode option to
>> 'channel'.
>>
>> Example:
>> map 0 0 0 0 1 2 2 3 queues 2@0 2@2 1@4 1@5\
>> hw 1 mode channel
>>
>> qdisc mqprio 8038: root tc 4 map 0 0 0 0 1 2 2 3 0 0 0 0 0 0 0 0
>> queues:(0:1) (2:3) (4:4) (5:5)
>> mode:channel
>> shaper:dcb
>>
>> The HW channels created are removed and all the queue configuration
>> is set to default when the qdisc is detached from the root of the
>> device.
>>
>> This patch also disables setting up channels via ethtool (ethtool -L)
>> when the TCs are configured using mqprio scheduler.
>>
>> The patch also limits setting ethtool Rx flow hash indirection
>> (ethtool -X eth0 equal N) to max queues configured via mqprio.
>> The Rx flow hash indirection input through ethtool should be
>> validated so that it is within in the queue range configured via
>> tc/mqprio. The bound checking is achieved by reporting the current
>> rss size to the kernel when queues are configured via mqprio.
>>
>> Example:
>> map 0 0 0 1 0 2 3 0 queues 2@0 4@2 8@6 11@14\
>> hw 1 mode channel
>>
>> Cannot set RX flow hash configuration: Invalid argument
>>
>> Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
>> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> drivers/net/ethernet/intel/i40e/i40e.h | 3 +
>> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 8 +-
>> drivers/net/ethernet/intel/i40e/i40e_main.c | 457 +++++++++++++++++++------
>> 3 files changed, 362 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
>> index bde982541772..024c88474951 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
>> @@ -54,6 +54,7 @@
>> #include <linux/clocksource.h>
>> #include <linux/net_tstamp.h>
>> #include <linux/ptp_clock_kernel.h>
>> +#include <net/pkt_cls.h>
>> #include "i40e_type.h"
>> #include "i40e_prototype.h"
>> #include "i40e_client.h"
>> @@ -700,6 +701,7 @@ struct i40e_vsi {
>> enum i40e_vsi_type type; /* VSI type, e.g., LAN, FCoE, etc */
>> s16 vf_id; /* Virtual function ID for SRIOV VSIs */
>>
>> + struct tc_mqprio_qopt_offload mqprio_qopt; /* queue parameters */
>> struct i40e_tc_configuration tc_config;
>> struct i40e_aqc_vsi_properties_data info;
>>
>> @@ -725,6 +727,7 @@ struct i40e_vsi {
>> u16 cnt_q_avail; /* num of queues available for channel usage */
>> u16 orig_rss_size;
>> u16 current_rss_size;
>> + bool reconfig_rss;
>>
>> u16 next_base_queue; /* next queue to be used for channel setup */
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> index afd3ca8d9851..72d5f2cdf419 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> @@ -2652,7 +2652,7 @@ static int i40e_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
>>
>> switch (cmd->cmd) {
>> case ETHTOOL_GRXRINGS:
>> - cmd->data = vsi->num_queue_pairs;
>> + cmd->data = vsi->rss_size;
>> ret = 0;
>> break;
>> case ETHTOOL_GRXFH:
>> @@ -3897,6 +3897,12 @@ static int i40e_set_channels(struct net_device *dev,
>> if (vsi->type != I40E_VSI_MAIN)
>> return -EINVAL;
>>
>> + /* We do not support setting channels via ethtool when TCs are
>> + * configured through mqprio
>> + */
>> + if (pf->flags & I40E_FLAG_TC_MQPRIO)
>> + return -EINVAL;
>> +
>> /* verify they are not requesting separate vectors */
>> if (!count || ch->rx_count || ch->tx_count)
>> return -EINVAL;
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index e23105bee6d1..e803aa1552c6 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -1588,6 +1588,170 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
>> return 0;
>> }
>>
>> +/**
>> + * i40e_config_rss_aq - Prepare for RSS using AQ commands
>> + * @vsi: vsi structure
>> + * @seed: RSS hash seed
>> + **/
>
> [...]
>
>> + * i40e_vsi_set_default_tc_config - set default values for tc configuration
>> + * @vsi: the VSI being configured
>> + **/
>> +static void i40e_vsi_set_default_tc_config(struct i40e_vsi *vsi)
>> +{
>> + u16 qcount;
>> + int i;
>> +
>> + /* Only TC0 is enabled */
>> + vsi->tc_config.numtc = 1;
>> + vsi->tc_config.enabled_tc = 1;
>> + qcount = min_t(int, vsi->alloc_queue_pairs,
>> + i40e_pf_get_max_q_per_tc(vsi->back));
>> + for (i = 0; i < I40E_MAX_TRAFFIC_CLASS; i++) {
>> + /* For the TC that is not enabled set the offset to to default
>> + * queue and allocate one queue for the given TC.
>> + */
>> + vsi->tc_config.tc_info[i].qoffset = 0;
>> + if (i == 0)
>> + vsi->tc_config.tc_info[i].qcount = qcount;
>> + else
>> + vsi->tc_config.tc_info[i].qcount = 1;
>> + vsi->tc_config.tc_info[i].netdev_tc = 0;
>> + }
>> +}
>> +
>> /**
>> * i40e_setup_tc - configure multiple traffic classes
>> * @netdev: net device to configure
>> - * @tc: number of traffic classes to enable
>> + * @type_data: tc offload data
>> **/
>> -static int i40e_setup_tc(struct net_device *netdev, u8 tc)
>> +static int i40e_setup_tc(struct net_device *netdev, void *type_data)
>> {
>> + struct tc_mqprio_qopt_offload *mqprio_qopt = type_data;
>> struct i40e_netdev_priv *np = netdev_priv(netdev);
>> struct i40e_vsi *vsi = np->vsi;
>> struct i40e_pf *pf = vsi->back;
>> - u8 enabled_tc = 0;
>> + u8 enabled_tc = 0, num_tc, hw;
>> + bool need_reset = false;
>> int ret = -EINVAL;
>> + u16 mode;
>> int i;
>>
>> - /* Check if DCB enabled to continue */
>> - if (!(pf->flags & I40E_FLAG_DCB_ENABLED)) {
>> - netdev_info(netdev, "DCB is not enabled for adapter\n");
>> - goto exit;
>> + num_tc = mqprio_qopt->qopt.num_tc;
>> + hw = mqprio_qopt->qopt.hw;
>> + mode = mqprio_qopt->mode;
>> + if (!hw) {
>
> When stack call the ndo_setup_tc, then qopt.hw is always non-zero, Can you
> tell me why you need to check for this?
This needs to be checked here because when the qdisc is detached from
root of the device, i.e. 'tc qdisc del dev <interface>', the 'mqprio
destroy' flow calls ndo_setup_tc with the offload values zeroed out for
proper clean up.
-Amritha
>
> Thanks,
> Yunsheng Lin
>
>> + pf->flags &= ~I40E_FLAG_TC_MQPRIO;
>> + memcpy(&vsi->mqprio_qopt, mqprio_qopt, sizeof(*mqprio_qopt));
>> + goto config_tc;
>> }
>>
>> /* Check if MFP enabled */
>> if (pf->flags & I40E_FLAG_MFP_ENABLED) {
>> - netdev_info(netdev, "Configuring TC not supported in MFP mode\n");
>> - goto exit;
>> + netdev_info(netdev,
>> + "Configuring TC not supported in MFP mode\n");
>> + return ret;
>> }
>> + switch (mode) {
>> + case TC_MQPRIO_MODE_DCB:
>> + pf->flags &= ~I40E_FLAG_TC_MQPRIO;
>>
>> - /* Check whether tc count is within enabled limit */
>> - if (tc > i40e_pf_get_num_tc(pf)) {
>> - netdev_info(netdev, "TC count greater than enabled on link for adapter\n");
>> - goto exit;
>> + /* Check if DCB enabled to continue */
>> + if (!(pf->flags & I40E_FLAG_DCB_ENABLED)) {
>> + netdev_info(netdev,
>> + "DCB is not enabled for adapter\n");
>> + return ret;
>> + }
>> +
>> + /* Check whether tc count is within enabled limit */
>> + if (num_tc > i40e_pf_get_num_tc(pf)) {
>> + netdev_info(netdev,
>> + "TC count greater than enabled on link for adapter\n");
>> + return ret;
>> + }
>> + break;
>> + case TC_MQPRIO_MODE_CHANNEL:
>> + if (pf->flags & I40E_FLAG_DCB_ENABLED) {
>> + netdev_info(netdev,
>> + "Full offload of TC Mqprio options is not supported when DCB is enabled\n");
>> + return ret;
>> + }
>> + if (!(pf->flags & I40E_FLAG_MSIX_ENABLED))
>> + return ret;
>> + ret = i40e_validate_mqprio_qopt(vsi, mqprio_qopt);
>> + if (ret)
>> + return ret;
>> + memcpy(&vsi->mqprio_qopt, mqprio_qopt,
>> + sizeof(*mqprio_qopt));
>> + pf->flags |= I40E_FLAG_TC_MQPRIO;
>> + pf->flags &= ~I40E_FLAG_DCB_ENABLED;
>> + break;
>> + default:
>> + return -EINVAL;
>> }
>>
>> +config_tc:
>> /* Generate TC map for number of tc requested */
>> - for (i = 0; i < tc; i++)
>> + for (i = 0; i < num_tc; i++)
>> enabled_tc |= BIT(i);
>>
>> /* Requesting same TC configuration as already enabled */
>> - if (enabled_tc == vsi->tc_config.enabled_tc)
>> + if (enabled_tc == vsi->tc_config.enabled_tc &&
>> + mode != TC_MQPRIO_MODE_CHANNEL)
>> return 0;
>>
>> /* Quiesce VSI queues */
>> i40e_quiesce_vsi(vsi);
>>
>> + if (!hw && !(pf->flags & I40E_FLAG_TC_MQPRIO))
>> + i40e_remove_queue_channels(vsi);
>> +
>> /* Configure VSI for enabled TCs */
>> ret = i40e_vsi_config_tc(vsi, enabled_tc);
>> if (ret) {
>> netdev_info(netdev, "Failed configuring TC for VSI seid=%d\n",
>> vsi->seid);
>> + need_reset = true;
>> goto exit;
>> }
>>
>> @@ -6272,11 +6595,18 @@ static int i40e_setup_tc(struct net_device *netdev, u8 tc)
>> if (ret) {
>> netdev_info(netdev,
>> "Failed configuring queue channels\n");
>> + need_reset = true;
>> goto exit;
>> }
>> }
>>
>> exit:
>> + /* Reset the configuration data to defaults, only TC0 is enabled */
>> + if (need_reset) {
>> + i40e_vsi_set_default_tc_config(vsi);
>> + need_reset = false;
>> + }
>> +
>> /* Unquiesce VSI */
>> i40e_unquiesce_vsi(vsi);
>> return ret;
>> @@ -6285,14 +6615,10 @@ static int i40e_setup_tc(struct net_device *netdev, u8 tc)
>> static int __i40e_setup_tc(struct net_device *netdev, enum tc_setup_type type,
>> void *type_data)
>> {
>> - struct tc_mqprio_qopt *mqprio = type_data;
>> -
>> if (type != TC_SETUP_MQPRIO)
>> return -EOPNOTSUPP;
>>
>> - mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
>> -
>> - return i40e_setup_tc(netdev, mqprio->num_tc);
>> + return i40e_setup_tc(netdev, type_data);
>> }
>>
>> /**
>> @@ -9153,45 +9479,6 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf)
>> return err;
>> }
>>
>> -/**
>> - * i40e_config_rss_aq - Prepare for RSS using AQ commands
>> - * @vsi: vsi structure
>> - * @seed: RSS hash seed
>> - **/
>> -static int i40e_config_rss_aq(struct i40e_vsi *vsi, const u8 *seed,
>> - u8 *lut, u16 lut_size)
>> -{
>> - struct i40e_pf *pf = vsi->back;
>> - struct i40e_hw *hw = &pf->hw;
>> - int ret = 0;
>> -
>> - if (seed) {
>> - struct i40e_aqc_get_set_rss_key_data *seed_dw =
>> - (struct i40e_aqc_get_set_rss_key_data *)seed;
>> - ret = i40e_aq_set_rss_key(hw, vsi->id, seed_dw);
>> - if (ret) {
>> - dev_info(&pf->pdev->dev,
>> - "Cannot set RSS key, err %s aq_err %s\n",
>> - i40e_stat_str(hw, ret),
>> - i40e_aq_str(hw, hw->aq.asq_last_status));
>> - return ret;
>> - }
>> - }
>> - if (lut) {
>> - bool pf_lut = vsi->type == I40E_VSI_MAIN ? true : false;
>> -
>> - ret = i40e_aq_set_rss_lut(hw, vsi->id, pf_lut, lut, lut_size);
>> - if (ret) {
>> - dev_info(&pf->pdev->dev,
>> - "Cannot set RSS lut, err %s aq_err %s\n",
>> - i40e_stat_str(hw, ret),
>> - i40e_aq_str(hw, hw->aq.asq_last_status));
>> - return ret;
>> - }
>> - }
>> - return ret;
>> -}
>> -
>> /**
>> * i40e_get_rss_aq - Get RSS keys and lut by using AQ commands
>> * @vsi: Pointer to vsi structure
>> @@ -9238,46 +9525,6 @@ static int i40e_get_rss_aq(struct i40e_vsi *vsi, const u8 *seed,
>> return ret;
>> }
>>
>> -/**
>> - * i40e_vsi_config_rss - Prepare for VSI(VMDq) RSS if used
>> - * @vsi: VSI structure
>> - **/
>> -static int i40e_vsi_config_rss(struct i40e_vsi *vsi)
>> -{
>> - u8 seed[I40E_HKEY_ARRAY_SIZE];
>> - struct i40e_pf *pf = vsi->back;
>> - u8 *lut;
>> - int ret;
>> -
>> - if (!(pf->hw_features & I40E_HW_RSS_AQ_CAPABLE))
>> - return 0;
>> -
>> - if (!vsi->rss_size)
>> - vsi->rss_size = min_t(int, pf->alloc_rss_size,
>> - vsi->num_queue_pairs);
>> - if (!vsi->rss_size)
>> - return -EINVAL;
>> -
>> - lut = kzalloc(vsi->rss_table_size, GFP_KERNEL);
>> - if (!lut)
>> - return -ENOMEM;
>> - /* Use the user configured hash keys and lookup table if there is one,
>> - * otherwise use default
>> - */
>> - if (vsi->rss_lut_user)
>> - memcpy(lut, vsi->rss_lut_user, vsi->rss_table_size);
>> - else
>> - i40e_fill_rss_lut(pf, lut, vsi->rss_table_size, vsi->rss_size);
>> - if (vsi->rss_hkey_user)
>> - memcpy(seed, vsi->rss_hkey_user, I40E_HKEY_ARRAY_SIZE);
>> - else
>> - netdev_rss_key_fill((void *)seed, I40E_HKEY_ARRAY_SIZE);
>> - ret = i40e_config_rss_aq(vsi, seed, lut, vsi->rss_table_size);
>> - kfree(lut);
>> -
>> - return ret;
>> -}
>> -
>> /**
>> * i40e_config_rss_reg - Configure RSS keys and lut by writing registers
>> * @vsi: Pointer to vsi structure
>>
>
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-16 16:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <20171016180442-mutt-send-email-mst@kernel.org>
On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>> >> +static int virtnet_reset(struct virtnet_info *vi)
>> >> +{
>> >> + struct virtio_device *dev = vi->vdev;
>> >> + int ret;
>> >> +
>> >> + virtio_config_disable(dev);
>> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> >> + virtnet_freeze_down(dev, true);
>> >> + remove_vq_common(vi);
>> >> +
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> >> +
>> >> + ret = virtio_finalize_features(dev);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + ret = virtnet_restore_up(dev);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> >> + virtio_config_enable(dev);
>> >> + return 0;
>> >> +
>> >> +err:
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> >> + return ret;
>> >> +}
>> >> +
>> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> >> {
>> >> struct scatterlist sg;
>> >
>> > I have a question here though. How do things like MAC address
>> > get restored?
>> >
>> > What about the rx mode?
>> >
>> > vlans?
>>
>> The function as is releases and reinitializes only ring state.
>> Device configuration such as mac and vlan persist across
>> the reset.
>
> What gave you this impression? Take a look at e.g. this
> code in qemu:
>
> static void virtio_net_reset(VirtIODevice *vdev)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
>
> /* Reset back to compatibility mode */
> n->promisc = 1;
> n->allmulti = 0;
> n->alluni = 0;
> n->nomulti = 0;
> n->nouni = 0;
> n->nobcast = 0;
> /* multiqueue is disabled by default */
> n->curr_queues = 1;
> timer_del(n->announce_timer);
> n->announce_counter = 0;
> n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>
> /* Flush any MAC and VLAN filter table state */
> n->mac_table.in_use = 0;
> n->mac_table.first_multi = 0;
> n->mac_table.multi_overflow = 0;
> n->mac_table.uni_overflow = 0;
> memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> memset(n->vlans, 0, MAX_VLAN >> 3);
> }
>
> So device seems to lose all state, you have to re-program it.
Oh, indeed! The guest does not reset its state, so it might
be out of sync with the host after the operation. Was this not
an issue when previously resetting in the context of xdp?
>> > Also, it seems that LINK_ANNOUNCE requests will get ignored
>> > even if they got set before the reset, leading to downtime.
>>
>> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
>> requests? That flag is tested and netdev_notify_peers
>> called before resetting virtio ring state.
>
> Yes but I wonder if there's a race where announce
> is set after it is read but before NEED_RESET is read.
>
> Re-reading status from the config before reset
> might be necessary.
Thanks, I'll have a look. Perhaps a host should simply not
request a reset while it is waiting for an announce ack.
^ permalink raw reply
* Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
From: Andrew Lunn @ 2017-10-16 16:09 UTC (permalink / raw)
To: Rodney Cummings
Cc: Vivien Didelot, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,
David S. Miller, Florian Fainelli, David Laight
In-Reply-To: <CY1PR0401MB1536F870727AFA21BFC31115924F0@CY1PR0401MB1536.namprd04.prod.outlook.com>
On Mon, Oct 16, 2017 at 03:23:42PM +0000, Rodney Cummings wrote:
> I am concerned about this proposed change.
>
> According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end
> station) that is internal to the bridge (i.e. switch) can use the
> same individual global MAC address that the switch is using. That
> behavior is common.
>
> Use of a local semi-random MAC address is dangerous on
> Ethernet. Local MAC addresses can only be safely used in very narrow
> use cases, most of which are Wi-Fi. Generally speaking, local MAC
> addresses can be duplicated in the network, which causes many
> Ethernet protocols to break down. Therefore, general-purpose
> implementations like DSA cannot use a local MAC address in a
> reliable manner.
Hi Rodney
This is interesting. So i did some research. The Marvell Switch only
uses this MAC address for Pause frames. Nothing else.
802.3-2015 Section 2, Annex 31B says:
The globally assigned 48-bit multicast address 01-80-C2-00-00-01 has
been reserved for use in MAC Control PAUSE frames for inhibiting
transmission of data frames from a DTE in a full duplex mode IEEE
802.3 LAN. IEEE 802.1D-conformant bridges will not forward frames
sent to this multicast destination address, regardless of the state
of the bridge’s ports, or whether or not the bridge
implements the MAC Control sublayer. To allow generic full duplex
flow control, stations implementing the PAUSE operation shall
instruct the MAC (e.g., through layer management) to enable
reception of frames with destination address equal to this multicast
address.
NOTE—By definition, an IEEE 802.3 LAN operating in full
duplex mode comprises exactly two stations, thus there is no
ambiguity regarding the destination DTE’s identity. The use
of a well-known multicast address relieves the MAC Control sublayer
and its client from having to know, and maintain knowledge of, the
individual 48-bit address of the other DTE in a full duplex
environment.
When MAC Control PFC operation (see Annex 31D and IEEE Std 802.1Q)
has been enabled, MAC Control PAUSE operation shall be disabled.
So, received Pause frames never leave the MAC. They don't get bridged,
nor do they get passed up for host processing. They are purely point
to point between two MAC peers. The destination is unambiguous. It is
simple the other MAC peer. The destination address makes it clear it
is a pause frame, the the source address seems to be unneeded.
In this context, a random MAC addresses are safe.
In the more general case, i would agree with you. Collisions are
possible, causing problems.
Andrew
^ permalink raw reply
* Re: [net-next 6/9] e1000e: fix buffer overrun while the I219 is processing DMA transactions
From: Alexander Duyck @ 2017-10-16 16:11 UTC (permalink / raw)
To: Neftin, Sasha
Cc: David Laight, Jeff Kirsher, davem@davemloft.net,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com
In-Reply-To: <b7d05918-8480-b8bf-fe1c-f9b6395e8293@intel.com>
On Mon, Oct 16, 2017 at 3:24 AM, Neftin, Sasha <sasha.neftin@intel.com> wrote:
> On 10/11/2017 12:07, David Laight wrote:
>>
>> From: Jeff Kirsher
>>>
>>> Sent: 10 October 2017 18:22
>>> Intel 100/200 Series Chipset platforms reduced the round-trip
>>> latency for the LAN Controller DMA accesses, causing in some high
>>> performance cases a buffer overrun while the I219 LAN Connected
>>> Device is processing the DMA transactions. I219LM and I219V devices
>>> can fall into unrecovered Tx hang under very stressfully UDP traffic
>>> and multiple reconnection of Ethernet cable. This Tx hang of the LAN
>>> Controller is only recovered if the system is rebooted. Slightly slow
>>> down DMA access by reducing the number of outstanding requests.
>>> This workaround could have an impact on TCP traffic performance
>>> on the platform. Disabling TSO eliminates performance loss for TCP
>>> traffic without a noticeable impact on CPU performance.
>>>
>>> Please, refer to I218/I219 specification update:
>>> https://www.intel.com/content/www/us/en/embedded/products/networking/
>>> ethernet-connection-i218-family-documentation.html
>>>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>> Reviewed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>> Reviewed-by: Raanan Avargil <raanan.avargil@intel.com>
>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>> drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index ee9de3500331..14b096f3d1da 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -3021,8 +3021,8 @@ static void e1000_configure_tx(struct e1000_adapter
>>> *adapter)
>>>
>>> hw->mac.ops.config_collision_dist(hw);
>>>
>>> - /* SPT and CNP Si errata workaround to avoid data corruption */
>>> - if (hw->mac.type >= e1000_pch_spt) {
>>> + /* SPT and KBL Si errata workaround to avoid data corruption */
>>> + if (hw->mac.type == e1000_pch_spt) {
>>> u32 reg_val;
>>>
>>> reg_val = er32(IOSFPC);
>>> @@ -3030,7 +3030,9 @@ static void e1000_configure_tx(struct e1000_adapter
>>> *adapter)
>>> ew32(IOSFPC, reg_val);
>>>
>>> reg_val = er32(TARC(0));
>>> - reg_val |= E1000_TARC0_CB_MULTIQ_3_REQ;
>>> + /* SPT and KBL Si errata workaround to avoid Tx hang */
>>> + reg_val &= ~BIT(28);
>>> + reg_val |= BIT(29);
>>
>> Shouldn't some more of the commit message about what this is doing
>> be in the comment?
>
> There is provided link on specification update:
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/i218-i219-ethernet-connection-spec-update.pdf?asset=9561.
> This is Intel's public edition.
>>
>> And shouldn't the 28 and 28 be named constants?
>
> (28 and 29) you can easy understand from the code that value has been
> changed from 3 to 2. There is no point add flags here I thought.
I have to agree with David. This isn't clear and this is going in the
opposite direction of being clear towards being very murky.
You already had the E1000_TARC0_CB_MULTIQ_3_REQ define. It shouldn't
be hard to come up with a bitmask that defines the full width of the
field you are updating so that you can use that mask to clear out the
value, and then also define a value for "MULTIQ_2_REQ" to replace it
the value you were using before. Assuming we still want to go with
this route.
He also has a point about using netif_set_gso_max_size() to restrict
the GSO size. If that would work for something like this then that
might be the preferred way to go as you wouldn't be introducing the
same type of issues as you currently do in that you are requiring
disabling TSO in order to avoid "performance loss" which in this case
I assume you are only referring to throughput without taking CPU into
account.
- Alex
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: TCP experimental option for SMC
From: Eric Dumazet @ 2017-10-16 16:13 UTC (permalink / raw)
To: Ursula Braun
Cc: davem, netdev, linux-s390, jwi, schwidefsky, heiko.carstens,
raspl
In-Reply-To: <20171016154202.72635-2-ubraun@linux.vnet.ibm.com>
On Mon, 2017-10-16 at 17:42 +0200, Ursula Braun wrote:
...
> +static void smc_set_capability(struct inet_request_sock *ireq,
> + const struct tcp_options_received *rx_opt)
> +{
> +#if IS_ENABLED(CONFIG_SMC)
> + if (!static_key_false(&tcp_have_smc))
> + return;
> +
> + if (rx_opt->smc_ok)
> + ireq->smc_ok = 1;
> + else
> + ireq->smc_ok = 0;
> +#endif
> +}
I do not think this static_key_false(&tcp_have_smc)) makes sense here.
1) If you have a SYN packet while the key is not set
2) Then tcp_have_smc is changed
3) ACK packets comes, and finds random value in ireq->smc_ok
So really here you have to set this field.
static void smc_set_capability(struct inet_request_sock *ireq,
const struct tcp_options_received *rx_opt)
{
#if IS_ENABLED(CONFIG_SMC)
ireq->smc_ok = rx_opt->smc_ok;
#endif
}
^ permalink raw reply
* Re: [PATCH net] bpf: disallow arithmetic operations on context pointer
From: Edward Cree @ 2017-10-16 16:16 UTC (permalink / raw)
To: Jakub Kicinski, netdev; +Cc: oss-drivers, alexei.starovoitov, daniel
In-Reply-To: <20171016154552.30640-1-jakub.kicinski@netronome.com>
On 16/10/17 16:45, Jakub Kicinski wrote:
> Commit f1174f77b50c ("bpf/verifier: rework value tracking")
> removed the crafty selection of which pointer types are
> allowed to be modified. This is OK for most pointer types
> since adjust_ptr_min_max_vals() will catch operations on
> immutable pointers. One exception is PTR_TO_CTX which is
> now allowed to be offseted freely.
>
> The intent of aforementioned commit was to allow context
> access via modified registers. The offset passed to
> ->is_valid_access() verifier callback has been adjusted
> by the value of the variable offset.
>
> What is missing, however, is taking the variable offset
> into account when the context register is used. Or in terms
> of the code adding the offset to the value passed to the
> ->convert_ctx_access() callback. This leads to the following
Good catch.
So the problem is just that convert_ctx_access() can't deal
with it, yes? Assuming that the offset is constant, because
otherwise we'd reject it anyway, we _could_ stash that offset
in insn_aux_data, and reject any paths that tried to change it
subsequently; and then convert_ctx_access() could be given the
total (off + reg->off) and then the reg->off could be subtracted
from the result, giving the right 'converted' insn offset.
That would then mean that your example,
> eBPF user code:
>
> r1 += 68
> r0 = *(u32 *)(r1 + 8)
> exit
>
> being translated to this in kernel space:
>
> 0: (07) r1 += 68
> 1: (61) r0 = *(u32 *)(r1 +180)
> 2: (95) exit
would instead convert offset 76 to something else, let's say
200 just for the sake of argument, and produce
1: (61) r0 = *(u32 *)(r1 + 132)
which would then do the right thing at run time.
However, I don't know whether anyone would actually want this
to be supported for their programs, and so I'm happy to
disallow this for net and then maybe we can follow up in
net-next with the change I describe above if it's useful.
> Offset 8 is corresponding to 180 in the kernel, but offset
> 76 is valid too. Verifier will "accept" access to offset
> 68+8=76 but then "convert" access to offset 8 as 180.
> Effective access to offset 248 is beyond the kernel context.
> (This is a __sk_buff example on a debug-heavy kernel -
> packet mark is 8 -> 180, 76 would be data.)
>
> Dereferencing the modified context pointer is not as easy
> as dereferencing other types, because we have to translate
> the access to reading a field in kernel structures which is
> usually at a different offset and often of a different size.
> To allow modifying the pointer we would have to make sure
> that given eBPF instruction will always access the same
> field or the fields accessed are "compatible" in terms of
> offset and size...
>
> Disallow dereferencing modified context pointers and add
> to selftests the test case described here.
>
> Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> Dave, a merge note - in net-next this will need env to be passed
> to verbose().
>
> kernel/bpf/verifier.c | 8 ++++++--
> tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8b8d6ba39e23..8499759d0c7a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> /* ctx accesses must be at a fixed offset, so that we can
> * determine what type of data were returned.
> */
> - if (!tnum_is_const(reg->var_off)) {
> + if (reg->off) {
> + verbose("derefence of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
This is slightly unclear, it's not that two adds is bad (e.g. r1 += 8;
r0 = *(u32 *)r1 is bad too), it's that the offset must be in the load,
not the register; your message might be accurate for some compilers but
not in full generality (especially for assemblers without compiling).
Also, sp. "dereference".
> + regno, reg->off, off - reg->off);
> + return -EACCES;
> + }
> + if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
> char tn_buf[48];
>
> tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
> @@ -1124,7 +1129,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> tn_buf, off, size);
> return -EACCES;
> }
> - off += reg->var_off.value;
> err = check_ctx_access(env, insn_idx, off, size, t, ®_type);
> if (!err && t == BPF_READ && value_regno >= 0) {
> /* ctx access returns either a scalar, or a
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 26f3250bdcd2..d41c77e7b39b 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -6645,6 +6645,20 @@ static struct bpf_test tests[] = {
> .errstr = "BPF_END uses reserved fields",
> .result = REJECT,
> },
> + {
> + "arithmetic ops make PTR_TO_CTX unusable",
> + .insns = {
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1,
> + offsetof(struct __sk_buff, data) -
> + offsetof(struct __sk_buff, mark)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
> + offsetof(struct __sk_buff, mark)),
> + BPF_EXIT_INSN(),
> + },
> + .errstr = "derefence of modified ctx ptr R1 off=68+8, ctx+const is allowed, ctx+const+const is not",
> + .result = REJECT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> };
>
> static int probe_filter_length(const struct bpf_insn *fp)
^ permalink raw reply
* Re: [iproute2] regression in ss output
From: Stephen Hemminger @ 2017-10-16 16:20 UTC (permalink / raw)
To: Humberto Alves; +Cc: netdev@vger.kernel.org
In-Reply-To: <AM4PR0901MB125261BF480B49D2BA2DF46BCC480@AM4PR0901MB1252.eurprd09.prod.outlook.com>
On Fri, 13 Oct 2017 09:57:37 +0000
Humberto Alves <hjalves@live.com> wrote:
> Hi! With the last iproute2 release, ss command output does not
> differentiate between any-address IPv4 sockets from the IPv6 ones.
> I don't know if this is an expected behaviour, but the old output looks
> more useful. Below I pasted the difference between the old behaviour and
> the new one.
> I am happy to send a patch if you want. Thanks
>
>
> iproute2-4.12
> -------------
>
> $ ss --version
> ss utility, iproute2-ss170705
>
> $ ss -ntl
> State Recv-Q Send-Q Local Address:Port Peer
> Address:Port
> LISTEN 0 100 *:587 *:*
>
> LISTEN 0 100 *:110 *:*
>
> LISTEN 0 100 *:143 *:*
>
> LISTEN 0 128 *:80 *:*
>
> LISTEN 0 128 *:2225 *:*
>
> LISTEN 0 10 127.0.0.1:5010 *:*
>
> LISTEN 0 128 *:20 *:*
>
> LISTEN 0 128 127.0.0.1:20150 *:*
>
> LISTEN 0 128 *:61719 *:*
>
> LISTEN 0 100 *:25 *:*
>
> LISTEN 0 3 *:1723 *:*
>
> LISTEN 0 128 *:17500 *:*
>
> LISTEN 0 128 127.0.0.1:17600 *:*
>
> LISTEN 0 128 *:8000 *:*
>
> LISTEN 0 100 *:993 *:*
>
> LISTEN 0 128 127.0.0.1:17603 *:*
>
> LISTEN 0 100 *:995 *:*
>
> LISTEN 0 100 :::587 :::*
>
> LISTEN 0 100 :::110 :::*
>
> LISTEN 0 100 :::143 :::*
>
> LISTEN 0 128 :::22000 :::*
>
> LISTEN 0 128 :::80 :::*
>
> LISTEN 0 128 :::2225 :::*
>
> LISTEN 0 128 :::20 :::*
>
> LISTEN 0 128 ::1:20150 :::*
>
> LISTEN 0 100 :::25 :::*
>
> LISTEN 0 128 :::17500 :::*
>
> LISTEN 0 128 :::8384 :::*
>
> LISTEN 0 100 :::993 :::*
>
> LISTEN 0 100 :::995 :::*
>
> iproute2-4.13
> -------------
>
> $ ss --version
> ss utility, iproute2-ss170905
>
> $ ss -ntl
> State Recv-Q Send-Q Local Address:Port Peer
> Address:Port
> LISTEN 0 100 *:587 *:*
>
> LISTEN 0 100 *:110 *:*
>
> LISTEN 0 100 *:143 *:*
>
> LISTEN 0 128 *:80 *:*
>
> LISTEN 0 128 *:2225 *:*
>
> LISTEN 0 10 127.0.0.1:5010 *:*
>
> LISTEN 0 128 *:20 *:*
>
> LISTEN 0 128 127.0.0.1:20150 *:*
>
> LISTEN 0 128 *:61719 *:*
>
> LISTEN 0 100 *:25 *:*
>
> LISTEN 0 3 *:1723 *:*
>
> LISTEN 0 128 *:17500 *:*
>
> LISTEN 0 128 127.0.0.1:17600 *:*
>
> LISTEN 0 128 *:8000 *:*
>
> LISTEN 0 100 *:993 *:*
>
> LISTEN 0 128 127.0.0.1:17603 *:*
>
> LISTEN 0 100 *:995 *:*
>
> LISTEN 0 100 *:587 *:*
>
> LISTEN 0 100 *:110 *:*
>
> LISTEN 0 100 *:143 *:*
>
> LISTEN 0 128 *:22000 *:*
>
> LISTEN 0 128 *:80 *:*
>
> LISTEN 0 128 *:2225 *:*
>
> LISTEN 0 128 *:20 *:*
>
> LISTEN 0 128 [::1]:20150 *:*
>
> LISTEN 0 100 *:25 *:*
>
> LISTEN 0 128 *:17500 *:*
>
> LISTEN 0 128 *:8384 *:*
>
> LISTEN 0 100 *:993 *:*
>
> LISTEN 0 100 *:995 *:*
All changes are not regressions.
Yes it was intentional because the outputing [::]:20 looked ugly and using * makes IPv4 and IPv6
look the same.
^ permalink raw reply
* Re: [PATCH iproute2 v2 0/4] Fix ip segfault when using --color switch
From: Stephen Hemminger @ 2017-10-16 16:24 UTC (permalink / raw)
To: Petr Vorel; +Cc: netdev, Julien Fortin
In-Reply-To: <20171013135719.18450-1-petr.vorel@gmail.com>
On Fri, 13 Oct 2017 15:57:15 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:
> Hi Stephen,
>
> I cleanup code not to use magic offsets. I kept it in separate commits,
> as it's clearer what happened.
>
> Petr Vorel (4):
> color: Fix ip segfault when using --color switch
> color: Fix another ip segfault when using --color switch
> color: Cleanup code to remove "magic" offset + 7
> color: Rename enum
>
> include/color.h | 3 +--
> include/json_print.h | 2 +-
> lib/color.c | 17 ++++++++++-------
> 3 files changed, 12 insertions(+), 10 deletions(-)
>
Applied, Thans Petr
^ permalink raw reply
* Re: [PATCH iproute2 1/1] tests: Revert back /bin/sh in shebang
From: Stephen Hemminger @ 2017-10-16 16:25 UTC (permalink / raw)
To: Petr Vorel; +Cc: netdev
In-Reply-To: <20171015095945.30878-1-petr.vorel@gmail.com>
On Sun, 15 Oct 2017 11:59:45 +0200
Petr Vorel <petr.vorel@gmail.com> wrote:
> This was added by mistake in commit ecd44e68
> ("tests: Remove bashisms (s/source/.)")
>
> Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> ---
> testsuite/tests/ip/route/add_default_route.t | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/testsuite/tests/ip/route/add_default_route.t b/testsuite/tests/ip/route/add_default_route.t
> index 0b566f1f..569ba1f8 100755
> --- a/testsuite/tests/ip/route/add_default_route.t
> +++ b/testsuite/tests/ip/route/add_default_route.t
> @@ -1,4 +1,4 @@
> -#!/bin/bash
> +#!/bin/sh
>
> . lib/generic.sh
>
Sure, applied
^ permalink raw reply
* Re: [patch net v2 3/4] selftests: Introduce a new script to generate tc batch file
From: Lucas Bates @ 2017-10-16 16:24 UTC (permalink / raw)
To: Chris Mi; +Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, davem
In-Reply-To: <1508152718-28726-4-git-send-email-chrism@mellanox.com>
On Mon, Oct 16, 2017 at 7:18 AM, Chris Mi <chrism@mellanox.com> wrote:
> # ./tdc_batch.py -h
> usage: tdc_batch.py [-h] [-n NUMBER] [-o] [-s] [-p] device file
>
> TC batch file generator
>
> positional arguments:
> device device name
> file batch file name
>
> optional arguments:
> -h, --help show this help message and exit
> -n NUMBER, --number NUMBER
> how many lines in batch file
> -o, --skip_sw skip_sw (offload), by default skip_hw
> -s, --share_action all filters share the same action
> -p, --prio all filters have different prio
>
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Lucas Bates <lucasb@mojatatu.com>
> ---
> tools/testing/selftests/tc-testing/tdc_batch.py | 62 +++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100755 tools/testing/selftests/tc-testing/tdc_batch.py
>
> diff --git a/tools/testing/selftests/tc-testing/tdc_batch.py b/tools/testing/selftests/tc-testing/tdc_batch.py
> new file mode 100755
> index 0000000..707c6bf
> --- /dev/null
> +++ b/tools/testing/selftests/tc-testing/tdc_batch.py
> @@ -0,0 +1,62 @@
> +#!/usr/bin/python3
> +
> +"""
> +tdc_batch.py - a script to generate TC batch file
> +
> +Copyright (C) 2017 Chris Mi <chrism@mellanox.com>
> +"""
> +
> +import argparse
> +
> +parser = argparse.ArgumentParser(description='TC batch file generator')
> +parser.add_argument("device", help="device name")
> +parser.add_argument("file", help="batch file name")
> +parser.add_argument("-n", "--number", type=int,
> + help="how many lines in batch file")
> +parser.add_argument("-o", "--skip_sw",
> + help="skip_sw (offload), by default skip_hw",
> + action="store_true")
> +parser.add_argument("-s", "--share_action",
> + help="all filters share the same action",
> + action="store_true")
> +parser.add_argument("-p", "--prio",
> + help="all filters have different prio",
> + action="store_true")
> +args = parser.parse_args()
> +
> +device = args.device
> +file = open(args.file, 'w')
> +
> +number = 1
> +if args.number:
> + number = args.number
> +
> +skip = "skip_hw"
> +if args.skip_sw:
> + skip = "skip_sw"
> +
> +share_action = ""
> +if args.share_action:
> + share_action = "index 1"
> +
> +prio = "prio 1"
> +if args.prio:
> + prio = ""
> + if number > 0x4000:
> + number = 0x4000
> +
> +index = 0
> +for i in range(0x100):
> + for j in range(0x100):
> + for k in range(0x100):
> + mac = ("%02x:%02x:%02x" % (i, j, k))
> + src_mac = "e4:11:00:" + mac
> + dst_mac = "e4:12:00:" + mac
> + cmd = ("filter add dev %s %s protocol ip parent ffff: flower %s "
> + "src_mac %s dst_mac %s action drop %s" %
> + (device, prio, skip, src_mac, dst_mac, share_action))
> + file.write("%s\n" % cmd)
> + index += 1
> + if index >= number:
> + file.close()
> + exit(0)
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [patch net v2 4/4] selftests: Introduce a new test case to tc testsuite
From: Lucas Bates @ 2017-10-16 16:25 UTC (permalink / raw)
To: Chris Mi; +Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, davem
In-Reply-To: <1508152718-28726-5-git-send-email-chrism@mellanox.com>
On Mon, Oct 16, 2017 at 7:18 AM, Chris Mi <chrism@mellanox.com> wrote:
> In this patchset, we fixed a tc bug. This patch adds the test case
> that reproduces the bug. To run this test case, user should specify
> an existing NIC device:
> # sudo ./tdc.py -d enp4s0f0
>
> This test case belongs to category "flower". If user doesn't specify
> a NIC device, the test cases belong to "flower" will not be run.
>
> In this test case, we create 1M filters and all filters share the same
> action. When destroying all filters, kernel should not panic. It takes
> about 18s to run it.
>
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
I'm a little wary about adding changes like these into tdc.py
directly; I don't think it's going to be sustainable in the long run.
Even the namespace creation I put in to the original version is too
specific and limiting.
There are some upcoming changes to tdc to help address these
particular issues. I'll ack this for now, thanks.
Acked-by: Lucas Bates <lucasb@mojatatu.com>
> ---
> .../tc-testing/tc-tests/filters/tests.json | 23 +++++++++++++++++++++-
> tools/testing/selftests/tc-testing/tdc.py | 20 +++++++++++++++----
> tools/testing/selftests/tc-testing/tdc_config.py | 2 ++
> 3 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> index c727b96..5fa02d8 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> @@ -17,5 +17,26 @@
> "teardown": [
> "$TC qdisc del dev $DEV1 ingress"
> ]
> + },
> + {
> + "id": "d052",
> + "name": "Add 1M filters with the same action",
> + "category": [
> + "filter",
> + "flower"
> + ],
> + "setup": [
> + "$TC qdisc add dev $DEV2 ingress",
> + "./tdc_batch.py $DEV2 $BATCH_FILE --share_action -n 1000000"
> + ],
> + "cmdUnderTest": "$TC -b $BATCH_FILE",
> + "expExitCode": "0",
> + "verifyCmd": "$TC actions list action gact",
> + "matchPattern": "action order 0: gact action drop.*index 1 ref 1000000 bind 1000000",
> + "matchCount": "1",
> + "teardown": [
> + "$TC qdisc del dev $DEV2 ingress",
> + "/bin/rm $BATCH_FILE"
> + ]
> }
> -]
> \ No newline at end of file
> +]
> diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
> index cd61b78..5f11f5d 100755
> --- a/tools/testing/selftests/tc-testing/tdc.py
> +++ b/tools/testing/selftests/tc-testing/tdc.py
> @@ -88,7 +88,7 @@ def prepare_env(cmdlist):
> exit(1)
>
>
> -def test_runner(filtered_tests):
> +def test_runner(filtered_tests, args):
> """
> Driver function for the unit tests.
>
> @@ -105,6 +105,8 @@ def test_runner(filtered_tests):
> for tidx in testlist:
> result = True
> tresult = ""
> + if "flower" in tidx["category"] and args.device == None:
> + continue
> print("Test " + tidx["id"] + ": " + tidx["name"])
> prepare_env(tidx["setup"])
> (p, procout) = exec_cmd(tidx["cmdUnderTest"])
> @@ -152,6 +154,10 @@ def ns_create():
> exec_cmd(cmd, False)
> cmd = 'ip -s $NS link set $DEV1 up'
> exec_cmd(cmd, False)
> + cmd = 'ip link set $DEV2 netns $NS'
> + exec_cmd(cmd, False)
> + cmd = 'ip -s $NS link set $DEV2 up'
> + exec_cmd(cmd, False)
>
>
> def ns_destroy():
> @@ -211,7 +217,8 @@ def set_args(parser):
> help='Execute the single test case with specified ID')
> parser.add_argument('-i', '--id', action='store_true', dest='gen_id',
> help='Generate ID numbers for new test cases')
> - return parser
> + parser.add_argument('-d', '--device',
> + help='Execute the test case in flower category')
> return parser
>
>
> @@ -225,6 +232,8 @@ def check_default_settings(args):
>
> if args.path != None:
> NAMES['TC'] = args.path
> + if args.device != None:
> + NAMES['DEV2'] = args.device
> if not os.path.isfile(NAMES['TC']):
> print("The specified tc path " + NAMES['TC'] + " does not exist.")
> exit(1)
> @@ -381,14 +390,17 @@ def set_operation_mode(args):
> if (len(alltests) == 0):
> print("Cannot find a test case with ID matching " + target_id)
> exit(1)
> - catresults = test_runner(alltests)
> + catresults = test_runner(alltests, args)
> print("All test results: " + "\n\n" + catresults)
> elif (len(target_category) > 0):
> + if (target_category == "flower") and args.device == None:
> + print("Please specify a NIC device (-d) to run category flower")
> + exit(1)
> if (target_category not in ucat):
> print("Specified category is not present in this file.")
> exit(1)
> else:
> - catresults = test_runner(testcases[target_category])
> + catresults = test_runner(testcases[target_category], args)
> print("Category " + target_category + "\n\n" + catresults)
>
> ns_destroy()
> diff --git a/tools/testing/selftests/tc-testing/tdc_config.py b/tools/testing/selftests/tc-testing/tdc_config.py
> index 0108737..b635251 100644
> --- a/tools/testing/selftests/tc-testing/tdc_config.py
> +++ b/tools/testing/selftests/tc-testing/tdc_config.py
> @@ -12,6 +12,8 @@ NAMES = {
> # Name of veth devices to be created for the namespace
> 'DEV0': 'v0p0',
> 'DEV1': 'v0p1',
> + 'DEV2': '',
> + 'BATCH_FILE': './batch.txt',
> # Name of the namespace to use
> 'NS': 'tcut'
> }
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: Linux 4.12+ memory leak on router with i40e NICs
From: Paweł Staszewski @ 2017-10-16 16:26 UTC (permalink / raw)
To: Pavlos Parissis, Alexander Duyck
Cc: Anders K. Pedersen | Cohaesio, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, alexander.h.duyck@intel.com
In-Reply-To: <b4995fc8-1420-fe96-26f7-ad5d986b7d39@gmail.com>
W dniu 2017-10-16 o 13:20, Pavlos Parissis pisze:
> On 15/10/2017 02:58 πμ, Alexander Duyck wrote:
>> Hi Pawel,
>>
>> To clarify is that Dave Miller's tree or Linus's that you are talking
>> about? If it is Dave's tree how long ago was it you pulled it since I
>> think the fix was just pushed by Jeff Kirsher a few days ago.
>>
>> The issue should be fixed in the following commit:
>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/drivers/net/ethernet/intel/i40e/i40e_txrx.c?id=2b9478ffc550f17c6cd8c69057234e91150f5972
>>
>
> Do you know when it is going to be available on net-next and linux-stable repos?
>
> Cheers,
> Pavlos
>
>
I will make some tests today night with "net" git tree where this patch
is included.
Starting from 0:00 CET
:)
^ permalink raw reply
* Re: [PATCH iproute2 net-next] ip: bridge_slave: add support for per-port group_fwd_mask
From: Stephen Hemminger @ 2017-10-16 16:27 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, roopa
In-Reply-To: <1507900373-9911-1-git-send-email-nikolay@cumulusnetworks.com>
On Fri, 13 Oct 2017 16:12:53 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> This patch adds the iproute2 support for getting and setting the
> per-port group_fwd_mask. It also tries to resolve the value into a more
> human friendly format by printing the known protocols instead of only
> the raw value.
> The man page is also updated with the new option.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Applied. Thanks.
^ permalink raw reply
* Re: [PATCH net-next v5 5/5] selinux: bpf: Add addtional check for bpf object file receive
From: Stephen Smalley @ 2017-10-16 16:34 UTC (permalink / raw)
To: Chenbo Feng, netdev
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, James Morris, Paul Moore, Chenbo Feng
In-Reply-To: <20171012205510.36028-6-chenbofeng.kernel@gmail.com>
On Thu, 2017-10-12 at 13:55 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 49
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 225740688ab7..81d6c01b8825 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> bpf_prog_array __rcu *progs,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d3e152e282d8..8bdb98aa7f34 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_map_show_fdinfo,
> #endif
> @@ -967,7 +967,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 12cf7de8cbed..ef7e5c1de640 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
> struct cred *cred,
> return inode_has_perm(cred, file_inode(file), av, &ad);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> /* Check whether a task can use an open file descriptor to
> access an inode in a given way. Check access to the
> descriptor itself, and then use dentry_has_perm to
> @@ -1845,6 +1849,12 @@ static int file_has_perm(const struct cred
> *cred,
> goto out;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(cred));
> + if (rc)
> + return rc;
> +#endif
> +
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> if (av)
> @@ -2165,6 +2175,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return 0;
>
> @@ -6288,6 +6304,39 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
> return av;
> }
>
> +/* This function will check the file pass through unix socket or
> binder to see
> + * if it is a bpf related object. And apply correspinding checks on
> the bpf
> + * object based on the type. The bpf maps and programs, not like
> other files and
> + * socket, are using a shared anonymous inode inside the kernel as
> their inode.
> + * So checking that inode cannot identify if the process have
> privilege to
> + * access the bpf object and that's why we have to add this
> additional check in
> + * selinux_file_receive and selinux_binder_transfer_files.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (file->f_op == &bpf_prog_fops) {
> + prog = file->private_data;
> + bpfsec = prog->aux->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_PROG,
> + BPF__PROG_RUN, NULL);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
My apologies, I should have caught this earlier, but you didn't update
the class/permission symbols used above when you re-spun patch 4/5 with
a single bpf class with distinct permissions defined.
Please, ensure that the entire series compiles and works before
sending.
Also, you likely should be cc'ing linux-security-module mailing list
and selinux mailing list; I thought you were doing so on earlier
versions.
> +
> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> {
> u32 sid = current_sid();
^ permalink raw reply
* RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
From: Rodney Cummings @ 2017-10-16 16:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vivien Didelot, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,
David S. Miller, Florian Fainelli, David Laight
In-Reply-To: <20171016160941.GA13339@lunn.ch>
Hi Andrew,
I may have misunderstood.
If this MAC address is the destination, a local random MAC address doesn't work. PAUSE is using one of the 802.1Q reserved destination MAC addresses (as does LLDP, MSTP, 1588, etc). PAUSE needs to use that specially assigned address (01-80-C2-00-00-01).
I assumed we were talking about the source MAC address. For the source MAC address, I would agree that a local random MAC address works in some narrow scenarios.
My concern is that for a source MAC address, a local random MAC address is not safe in all networks, because it has the potential for duplication. That topic has been discussed quite a bit in IEEE 802.
Rodney
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, October 16, 2017 11:10 AM
> To: Rodney Cummings <rodney.cummings@ni.com>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@savoirfairelinux.com; David S. Miller <davem@davemloft.net>;
> Florian Fainelli <f.fainelli@gmail.com>; David Laight
> <David.Laight@ACULAB.COM>
> Subject: Re: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
>
> On Mon, Oct 16, 2017 at 03:23:42PM +0000, Rodney Cummings wrote:
> > I am concerned about this proposed change.
> >
> > According to IEEE Std 802.1Q, a "higher layer entity" (i.e. end
> > station) that is internal to the bridge (i.e. switch) can use the
> > same individual global MAC address that the switch is using. That
> > behavior is common.
> >
> > Use of a local semi-random MAC address is dangerous on
> > Ethernet. Local MAC addresses can only be safely used in very narrow
> > use cases, most of which are Wi-Fi. Generally speaking, local MAC
> > addresses can be duplicated in the network, which causes many
> > Ethernet protocols to break down. Therefore, general-purpose
> > implementations like DSA cannot use a local MAC address in a
> > reliable manner.
>
> Hi Rodney
>
> This is interesting. So i did some research. The Marvell Switch only
> uses this MAC address for Pause frames. Nothing else.
>
> 802.3-2015 Section 2, Annex 31B says:
>
> The globally assigned 48-bit multicast address 01-80-C2-00-00-01 has
> been reserved for use in MAC Control PAUSE frames for inhibiting
> transmission of data frames from a DTE in a full duplex mode IEEE
> 802.3 LAN. IEEE 802.1D-conformant bridges will not forward frames
> sent to this multicast destination address, regardless of the state
> of the bridge’s ports, or whether or not the bridge
> implements the MAC Control sublayer. To allow generic full duplex
> flow control, stations implementing the PAUSE operation shall
> instruct the MAC (e.g., through layer management) to enable
> reception of frames with destination address equal to this multicast
> address.
>
> NOTE—By definition, an IEEE 802.3 LAN operating in full
> duplex mode comprises exactly two stations, thus there is no
> ambiguity regarding the destination DTE’s identity. The use
> of a well-known multicast address relieves the MAC Control sublayer
> and its client from having to know, and maintain knowledge of, the
> individual 48-bit address of the other DTE in a full duplex
> environment.
>
> When MAC Control PFC operation (see Annex 31D and IEEE Std 802.1Q)
> has been enabled, MAC Control PAUSE operation shall be disabled.
>
> So, received Pause frames never leave the MAC. They don't get bridged,
> nor do they get passed up for host processing. They are purely point
> to point between two MAC peers. The destination is unambiguous. It is
> simple the other MAC peer. The destination address makes it clear it
> is a pause frame, the the source address seems to be unneeded.
>
> In this context, a random MAC addresses are safe.
>
> In the more general case, i would agree with you. Collisions are
> possible, causing problems.
>
> Andrew
^ permalink raw reply
* RE: [PATCH net-next v3 0/5] net: dsa: remove .set_addr
From: David Laight @ 2017-10-16 16:30 UTC (permalink / raw)
To: 'Andrew Lunn', Rodney Cummings
Cc: Vivien Didelot, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com,
David S. Miller, Florian Fainelli
In-Reply-To: <20171016160941.GA13339@lunn.ch>
From: Andrew Lunn
> Sent: 16 October 2017 17:10
...
> So, received Pause frames never leave the MAC. They don't get bridged,
> nor do they get passed up for host processing. They are purely point
> to point between two MAC peers. The destination is unambiguous. It is
> simple the other MAC peer. The destination address makes it clear it
> is a pause frame, the the source address seems to be unneeded.
>
> In this context, a random MAC addresses are safe.
Is there any reason why a fixed value (say 00:00:00:00:00:00)
can't be used?
> In the more general case, i would agree with you. Collisions are
> possible, causing problems.
For IP MAC addresses only go as far as the first router.
So the duplicates would (typically) have to be within the same subnet.
This makes the chance of a duplicate random address unlikely.
(Unless you have an un-subnetted class A network consisting of
multiple 1km long coax segments connected by 1km long repeaters
making a single collision domain.)
David
^ permalink raw reply
* Re: [PATCH net] bpf: disallow arithmetic operations on context pointer
From: Jakub Kicinski @ 2017-10-16 16:30 UTC (permalink / raw)
To: Edward Cree; +Cc: netdev, oss-drivers, alexei.starovoitov, daniel
In-Reply-To: <44a8fb95-298b-8715-ce4e-60df60c86343@solarflare.com>
On Mon, 16 Oct 2017 17:16:24 +0100, Edward Cree wrote:
> On 16/10/17 16:45, Jakub Kicinski wrote:
> > Commit f1174f77b50c ("bpf/verifier: rework value tracking")
> > removed the crafty selection of which pointer types are
> > allowed to be modified. This is OK for most pointer types
> > since adjust_ptr_min_max_vals() will catch operations on
> > immutable pointers. One exception is PTR_TO_CTX which is
> > now allowed to be offseted freely.
> >
> > The intent of aforementioned commit was to allow context
> > access via modified registers. The offset passed to
> > ->is_valid_access() verifier callback has been adjusted
> > by the value of the variable offset.
> >
> > What is missing, however, is taking the variable offset
> > into account when the context register is used. Or in terms
> > of the code adding the offset to the value passed to the
> > ->convert_ctx_access() callback. This leads to the following
> Good catch.
> So the problem is just that convert_ctx_access() can't deal
> with it, yes? Assuming that the offset is constant, because
> otherwise we'd reject it anyway, we _could_ stash that offset
> in insn_aux_data, and reject any paths that tried to change it
> subsequently; and then convert_ctx_access() could be given the
> total (off + reg->off) and then the reg->off could be subtracted
> from the result, giving the right 'converted' insn offset.
> That would then mean that your example,
> > eBPF user code:
> >
> > r1 += 68
> > r0 = *(u32 *)(r1 + 8)
> > exit
> >
> > being translated to this in kernel space:
> >
> > 0: (07) r1 += 68
> > 1: (61) r0 = *(u32 *)(r1 +180)
> > 2: (95) exit
> would instead convert offset 76 to something else, let's say
> 200 just for the sake of argument, and produce
> 1: (61) r0 = *(u32 *)(r1 + 132)
> which would then do the right thing at run time.
>
> However, I don't know whether anyone would actually want this
> to be supported for their programs, and so I'm happy to
> disallow this for net and then maybe we can follow up in
> net-next with the change I describe above if it's useful.
That was my thinking too. We would have to stash the offset and make
sure that it doesn't change on different paths, but it's probably not
suitable for -net, and nobody uses that today (although Alexei
mentioned there may be valid use cases, especially in tracing).
> > Offset 8 is corresponding to 180 in the kernel, but offset
> > 76 is valid too. Verifier will "accept" access to offset
> > 68+8=76 but then "convert" access to offset 8 as 180.
> > Effective access to offset 248 is beyond the kernel context.
> > (This is a __sk_buff example on a debug-heavy kernel -
> > packet mark is 8 -> 180, 76 would be data.)
> >
> > Dereferencing the modified context pointer is not as easy
> > as dereferencing other types, because we have to translate
> > the access to reading a field in kernel structures which is
> > usually at a different offset and often of a different size.
> > To allow modifying the pointer we would have to make sure
> > that given eBPF instruction will always access the same
> > field or the fields accessed are "compatible" in terms of
> > offset and size...
> >
> > Disallow dereferencing modified context pointers and add
> > to selftests the test case described here.
> >
> > Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> > Dave, a merge note - in net-next this will need env to be passed
> > to verbose().
> >
> > kernel/bpf/verifier.c | 8 ++++++--
> > tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++
> > 2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8b8d6ba39e23..8499759d0c7a 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
> > /* ctx accesses must be at a fixed offset, so that we can
> > * determine what type of data were returned.
> > */
> > - if (!tnum_is_const(reg->var_off)) {
> > + if (reg->off) {
> > + verbose("derefence of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
> This is slightly unclear, it's not that two adds is bad (e.g. r1 += 8;
> r0 = *(u32 *)r1 is bad too), it's that the offset must be in the load,
> not the register; your message might be accurate for some compilers but
> not in full generality (especially for assemblers without compiling).
I'm happy to hear better suggestions :) I've spent quite a bit of time
scratching my head thinking how to phrase this best. The first
part of the message is general enough IMHO, the second is targeted
mostly at C developers.
> Also, sp. "dereference".
Argh. aspell doesn't know that word, added to local dict now.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox