Netdev List
 help / color / mirror / Atom feed
* [v2, 3/3] ptp_qoriq: support automatic configuration for ptp timer
From: Yangbo Lu @ 2018-08-01 10:05 UTC (permalink / raw)
  To: netdev, madalin.bucur, Richard Cochran, Rob Herring, Shawn Guo,
	David S . Miller
  Cc: devicetree, linuxppc-dev, linux-arm-kernel, linux-kernel,
	Yangbo Lu
In-Reply-To: <20180801100554.36634-1-yangbo.lu@nxp.com>

This patch is to support automatic configuration for ptp timer.
If required ptp dts properties are not provided, driver could
try to calculate a set of default configurations to initialize
the ptp timer. This makes the driver work for many boards which
don't have the required ptp dts properties in current kernel.
Also the users could set dts properties by themselves according
to their requirement.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- Dropped module_param.
---
 drivers/ptp/ptp_qoriq.c       |  111 +++++++++++++++++++++++++++++++++++++++-
 include/linux/fsl/ptp_qoriq.h |    6 ++-
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_qoriq.c b/drivers/ptp/ptp_qoriq.c
index a14c317..095c185 100644
--- a/drivers/ptp/ptp_qoriq.c
+++ b/drivers/ptp/ptp_qoriq.c
@@ -29,6 +29,7 @@
 #include <linux/of_platform.h>
 #include <linux/timex.h>
 #include <linux/slab.h>
+#include <linux/clk.h>
 
 #include <linux/fsl/ptp_qoriq.h>
 
@@ -317,6 +318,105 @@ static int ptp_qoriq_enable(struct ptp_clock_info *ptp,
 	.enable		= ptp_qoriq_enable,
 };
 
+/**
+ * qoriq_ptp_nominal_freq - calculate nominal frequency according to
+ *			    reference clock frequency
+ *
+ * @clk_src: reference clock frequency
+ *
+ * The nominal frequency is the desired clock frequency.
+ * It should be less than the reference clock frequency.
+ * It should be a factor of 1000MHz.
+ *
+ * Return the nominal frequency
+ */
+static u32 qoriq_ptp_nominal_freq(u32 clk_src)
+{
+	u32 remainder = 0;
+
+	clk_src /= 1000000;
+	remainder = clk_src % 100;
+	if (remainder) {
+		clk_src -= remainder;
+		clk_src += 100;
+	}
+
+	do {
+		clk_src -= 100;
+
+	} while (1000 % clk_src);
+
+	return clk_src * 1000000;
+}
+
+/**
+ * qoriq_ptp_auto_config - calculate a set of default configurations
+ *
+ * @qoriq_ptp: pointer to qoriq_ptp
+ * @node: pointer to device_node
+ *
+ * If below dts properties are not provided, this function will be
+ * called to calculate a set of default configurations for them.
+ *   "fsl,tclk-period"
+ *   "fsl,tmr-prsc"
+ *   "fsl,tmr-add"
+ *   "fsl,tmr-fiper1"
+ *   "fsl,tmr-fiper2"
+ *   "fsl,max-adj"
+ *
+ * Return 0 if success
+ */
+static int qoriq_ptp_auto_config(struct qoriq_ptp *qoriq_ptp,
+				 struct device_node *node)
+{
+	struct clk *clk;
+	u64 freq_comp;
+	u64 max_adj;
+	u32 nominal_freq;
+	u32 clk_src = 0;
+
+	qoriq_ptp->cksel = DEFAULT_CKSEL;
+
+	clk = of_clk_get(node, 0);
+	if (!IS_ERR(clk)) {
+		clk_src = clk_get_rate(clk);
+		clk_put(clk);
+	}
+
+	if (clk_src <= 100000000UL) {
+		pr_err("error reference clock value, or lower than 100MHz\n");
+		return -EINVAL;
+	}
+
+	nominal_freq = qoriq_ptp_nominal_freq(clk_src);
+	if (!nominal_freq)
+		return -EINVAL;
+
+	qoriq_ptp->tclk_period = 1000000000UL / nominal_freq;
+	qoriq_ptp->tmr_prsc = DEFAULT_TMR_PRSC;
+
+	/* Calculate initial frequency compensation value for TMR_ADD register.
+	 * freq_comp = ceil(2^32 / freq_ratio)
+	 * freq_ratio = reference_clock_freq / nominal_freq
+	 */
+	freq_comp = ((u64)1 << 32) * nominal_freq;
+	if (do_div(freq_comp, clk_src))
+		freq_comp++;
+
+	qoriq_ptp->tmr_add = freq_comp;
+	qoriq_ptp->tmr_fiper1 = DEFAULT_FIPER1_PERIOD - qoriq_ptp->tclk_period;
+	qoriq_ptp->tmr_fiper2 = DEFAULT_FIPER2_PERIOD - qoriq_ptp->tclk_period;
+
+	/* max_adj = 1000000000 * (freq_ratio - 1.0) - 1
+	 * freq_ratio = reference_clock_freq / nominal_freq
+	 */
+	max_adj = 1000000000ULL * (clk_src - nominal_freq);
+	max_adj = max_adj / nominal_freq - 1;
+	qoriq_ptp->caps.max_adj = max_adj;
+
+	return 0;
+}
+
 static int qoriq_ptp_probe(struct platform_device *dev)
 {
 	struct device_node *node = dev->dev.of_node;
@@ -332,7 +432,7 @@ static int qoriq_ptp_probe(struct platform_device *dev)
 	if (!qoriq_ptp)
 		goto no_memory;
 
-	err = -ENODEV;
+	err = -EINVAL;
 
 	qoriq_ptp->caps = ptp_qoriq_caps;
 
@@ -351,10 +451,14 @@ static int qoriq_ptp_probe(struct platform_device *dev)
 				 "fsl,tmr-fiper2", &qoriq_ptp->tmr_fiper2) ||
 	    of_property_read_u32(node,
 				 "fsl,max-adj", &qoriq_ptp->caps.max_adj)) {
-		pr_err("device tree node missing required elements\n");
-		goto no_node;
+		pr_warn("device tree node missing required elements, try automatic configuration\n");
+
+		if (qoriq_ptp_auto_config(qoriq_ptp, node))
+			goto no_config;
 	}
 
+	err = -ENODEV;
+
 	qoriq_ptp->irq = platform_get_irq(dev, 0);
 
 	if (qoriq_ptp->irq < 0) {
@@ -436,6 +540,7 @@ static int qoriq_ptp_probe(struct platform_device *dev)
 	release_resource(qoriq_ptp->rsrc);
 no_resource:
 	free_irq(qoriq_ptp->irq, qoriq_ptp);
+no_config:
 no_node:
 	kfree(qoriq_ptp);
 no_memory:
diff --git a/include/linux/fsl/ptp_qoriq.h b/include/linux/fsl/ptp_qoriq.h
index dc3dac4..c1f003a 100644
--- a/include/linux/fsl/ptp_qoriq.h
+++ b/include/linux/fsl/ptp_qoriq.h
@@ -127,9 +127,13 @@ struct qoriq_ptp_registers {
 
 
 #define DRIVER		"ptp_qoriq"
-#define DEFAULT_CKSEL	1
 #define N_EXT_TS	2
 
+#define DEFAULT_CKSEL		1
+#define DEFAULT_TMR_PRSC	2
+#define DEFAULT_FIPER1_PERIOD	1000000000
+#define DEFAULT_FIPER2_PERIOD	100000
+
 struct qoriq_ptp {
 	void __iomem *base;
 	struct qoriq_ptp_registers regs;
-- 
1.7.1

^ permalink raw reply related

* [v2, 2/3] powerpc/mpc85xx: add clocks property for fman ptp timer node
From: Yangbo Lu @ 2018-08-01 10:05 UTC (permalink / raw)
  To: netdev, madalin.bucur, Richard Cochran, Rob Herring, Shawn Guo,
	David S . Miller
  Cc: devicetree, linuxppc-dev, linux-arm-kernel, linux-kernel,
	Yangbo Lu
In-Reply-To: <20180801100554.36634-1-yangbo.lu@nxp.com>

This patch is to add clocks property for fman ptp timer node.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None.
---
 arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi   |    1 +
 arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi   |    1 +
 arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi  |    1 +
 arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi  |    1 +
 arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi |    1 +
 5 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi
index 6b124f7..9b6cf91 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi
@@ -100,4 +100,5 @@ ptp_timer0: ptp-timer@4fe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x4fe000 0x1000>;
 	interrupts = <96 2 0 0>;
+	clocks = <&clockgen 3 0>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi
index b80aaf5..e95c11f 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi
@@ -100,4 +100,5 @@ ptp_timer1: ptp-timer@5fe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x5fe000 0x1000>;
 	interrupts = <97 2 0 0>;
+	clocks = <&clockgen 3 1>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi
index d3720fd..d62b36c 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi
@@ -105,4 +105,5 @@ ptp_timer0: ptp-timer@4fe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x4fe000 0x1000>;
 	interrupts = <96 2 0 0>;
+	clocks = <&clockgen 3 0>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi
index ae34c20..3102324 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi
@@ -105,4 +105,5 @@ ptp_timer1: ptp-timer@5fe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x5fe000 0x1000>;
 	interrupts = <97 2 0 0>;
+	clocks = <&clockgen 3 1>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
index 02f2755..c90702b 100644
--- a/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
+++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi
@@ -93,4 +93,5 @@ ptp_timer0: ptp-timer@4fe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x4fe000 0x1000>;
 	interrupts = <96 2 0 0>;
+	clocks = <&clockgen 3 0>;
 };
-- 
1.7.1

^ permalink raw reply related

* [v2, 1/3] arm64: dts: fsl: add clocks property for fman ptp timer node
From: Yangbo Lu @ 2018-08-01 10:05 UTC (permalink / raw)
  To: netdev, madalin.bucur, Richard Cochran, Rob Herring, Shawn Guo,
	David S . Miller
  Cc: devicetree, linuxppc-dev, linux-arm-kernel, linux-kernel,
	Yangbo Lu

This patch is to add clocks property for fman ptp timer node.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None.
---
 arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi
index a56a408..4664c33 100644
--- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi
+++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi
@@ -80,4 +80,5 @@ ptp_timer0: ptp-timer@1afe000 {
 	compatible = "fsl,fman-ptp-timer";
 	reg = <0x0 0x1afe000 0x0 0x1000>;
 	interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&clockgen 3 0>;
 };
-- 
1.7.1

^ permalink raw reply related

* [PATCH net-next] rxrpc: remove redundant variables 'sp' and 'did_discard'
From: YueHaibing @ 2018-08-01  9:52 UTC (permalink / raw)
  To: davem, dhowells; +Cc: linux-kernel, netdev, linux-afs, YueHaibing

Variables 'sp' and 'did_discard' are being assigned,
but are never used,hence they are redundant and can be removed.

fix fllowing warning:

net/rxrpc/call_event.c:165:25: warning: variable 'sp' set but not used [-Wunused-but-set-variable]
net/rxrpc/conn_client.c:1054:7: warning: variable 'did_discard' set but not used [-Wunused-but-set-variable]

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/rxrpc/call_event.c  | 2 --
 net/rxrpc/conn_client.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index 2021041..8e7434e 100644
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -162,7 +162,6 @@ static void rxrpc_congestion_timeout(struct rxrpc_call *call)
  */
 static void rxrpc_resend(struct rxrpc_call *call, unsigned long now_j)
 {
-	struct rxrpc_skb_priv *sp;
 	struct sk_buff *skb;
 	unsigned long resend_at;
 	rxrpc_seq_t cursor, seq, top;
@@ -207,7 +206,6 @@ static void rxrpc_resend(struct rxrpc_call *call, unsigned long now_j)
 
 		skb = call->rxtx_buffer[ix];
 		rxrpc_see_skb(skb, rxrpc_skb_tx_seen);
-		sp = rxrpc_skb(skb);
 
 		if (anno_type == RXRPC_TX_ANNO_UNACK) {
 			if (ktime_after(skb->tstamp, max_age)) {
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 5736f64..e4bfbd7 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -1051,7 +1051,6 @@ void rxrpc_discard_expired_client_conns(struct work_struct *work)
 		container_of(work, struct rxrpc_net, client_conn_reaper);
 	unsigned long expiry, conn_expires_at, now;
 	unsigned int nr_conns;
-	bool did_discard = false;
 
 	_enter("");
 
@@ -1113,7 +1112,6 @@ void rxrpc_discard_expired_client_conns(struct work_struct *work)
 	 * If someone re-sets the flag and re-gets the ref, that's fine.
 	 */
 	rxrpc_put_connection(conn);
-	did_discard = true;
 	nr_conns--;
 	goto next;
 
-- 
2.7.0

^ permalink raw reply related

* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-08-01  9:52 UTC (permalink / raw)
  To: jasowang; +Cc: Linux Kernel Network Developers, virtualization, mst
In-Reply-To: <30e62749-3cbd-ae88-6582-c20087884b20@redhat.com>

On Wed, Aug 1, 2018 at 2:01 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年08月01日 11:00, xiangxia.m.yue@gmail.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Factor out generic busy polling logic and will be
> > used for in tx path in the next patch. And with the patch,
> > qemu can set differently the busyloop_timeout for rx queue.
> >
> > In the handle_tx, the busypoll will vhost_net_disable/enable_vq
> > because we have poll the sock. This can improve performance.
> > [This is suggested by Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>]
> >
> > And when the sock receive skb, we should queue the poll if necessary.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > ---
> >   drivers/vhost/net.c | 131 ++++++++++++++++++++++++++++++++++++----------------
> >   1 file changed, 91 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 32c1b52..5b45463 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -440,6 +440,95 @@ static void vhost_net_signal_used(struct vhost_net_virtqueue *nvq)
> >       nvq->done_idx = 0;
> >   }
> >
> > +static int sk_has_rx_data(struct sock *sk)
> > +{
> > +     struct socket *sock = sk->sk_socket;
> > +
> > +     if (sock->ops->peek_len)
> > +             return sock->ops->peek_len(sock);
> > +
> > +     return skb_queue_empty(&sk->sk_receive_queue);
> > +}
> > +
> > +static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
> > +                                       struct vhost_virtqueue *vq)
> > +{
> > +     if (!vhost_vq_avail_empty(&net->dev, vq)) {
> > +             vhost_poll_queue(&vq->poll);
> > +     } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> > +             vhost_disable_notify(&net->dev, vq);
> > +             vhost_poll_queue(&vq->poll);
> > +     }
> > +}
> > +
> > +static void vhost_net_busy_poll_check(struct vhost_net *net,
> > +                                   struct vhost_virtqueue *rvq,
> > +                                   struct vhost_virtqueue *tvq,
> > +                                   bool rx)
> > +{
> > +     struct socket *sock = rvq->private_data;
> > +
> > +     if (rx)
> > +             vhost_net_busy_poll_try_queue(net, tvq);
> > +     else if (sock && sk_has_rx_data(sock->sk))
> > +             vhost_net_busy_poll_try_queue(net, rvq);
> > +     else {
> > +             /* On tx here, sock has no rx data, so we
> > +              * will wait for sock wakeup for rx, and
> > +              * vhost_enable_notify() is not needed. */
>
> A possible case is we do have rx data but guest does not refill the rx
> queue. In this case we may lose notifications from guest.
Yes, should consider this case. thanks.
> > +     }
> > +}
> > +
> > +static void vhost_net_busy_poll(struct vhost_net *net,
> > +                             struct vhost_virtqueue *rvq,
> > +                             struct vhost_virtqueue *tvq,
> > +                             bool *busyloop_intr,
> > +                             bool rx)
> > +{
> > +     unsigned long busyloop_timeout;
> > +     unsigned long endtime;
> > +     struct socket *sock;
> > +     struct vhost_virtqueue *vq = rx ? tvq : rvq;
> > +
> > +     mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
> > +     vhost_disable_notify(&net->dev, vq);
> > +     sock = rvq->private_data;
> > +
> > +     busyloop_timeout = rx ? rvq->busyloop_timeout:
> > +                             tvq->busyloop_timeout;
> > +
> > +
> > +     /* Busypoll the sock, so don't need rx wakeups during it. */
> > +     if (!rx)
> > +             vhost_net_disable_vq(net, vq);
>
> Actually this piece of code is not a factoring out. I would suggest to
> add this in another patch, or on top of this series.
I will add this in another patch.
> > +
> > +     preempt_disable();
> > +     endtime = busy_clock() + busyloop_timeout;
> > +
> > +     while (vhost_can_busy_poll(endtime)) {
> > +             if (vhost_has_work(&net->dev)) {
> > +                     *busyloop_intr = true;
> > +                     break;
> > +             }
> > +
> > +             if ((sock && sk_has_rx_data(sock->sk) &&
> > +                  !vhost_vq_avail_empty(&net->dev, rvq)) ||
> > +                 !vhost_vq_avail_empty(&net->dev, tvq))
> > +                     break;
>
> Some checks were duplicated in vhost_net_busy_poll_check(). Need
> consider to unify them.
OK
> > +
> > +             cpu_relax();
> > +     }
> > +
> > +     preempt_enable();
> > +
> > +     if (!rx)
> > +             vhost_net_enable_vq(net, vq);
>
> No need to enable rx virtqueue, if we are sure handle_rx() will be
> called soon.
If we disable rx virtqueue in handle_tx and don't send packets from
guest anymore(handle_tx is not called), so we can wake up for sock rx.
so the network is broken.
> > +
> > +     vhost_net_busy_poll_check(net, rvq, tvq, rx);
>
> It looks to me just open code all check here is better and easier to be
> reviewed.
will be changed.
> Thanks
>
> > +
> > +     mutex_unlock(&vq->mutex);
> > +}
> > +
> >   static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
> >                                   struct vhost_net_virtqueue *nvq,
> >                                   unsigned int *out_num, unsigned int *in_num,
> > @@ -753,16 +842,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
> >       return len;
> >   }
> >
> > -static int sk_has_rx_data(struct sock *sk)
> > -{
> > -     struct socket *sock = sk->sk_socket;
> > -
> > -     if (sock->ops->peek_len)
> > -             return sock->ops->peek_len(sock);
> > -
> > -     return skb_queue_empty(&sk->sk_receive_queue);
> > -}
> > -
> >   static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
> >                                     bool *busyloop_intr)
> >   {
> > @@ -770,41 +849,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
> >       struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
> >       struct vhost_virtqueue *rvq = &rnvq->vq;
> >       struct vhost_virtqueue *tvq = &tnvq->vq;
> > -     unsigned long uninitialized_var(endtime);
> >       int len = peek_head_len(rnvq, sk);
> >
> > -     if (!len && tvq->busyloop_timeout) {
> > +     if (!len && rvq->busyloop_timeout) {
> >               /* Flush batched heads first */
> >               vhost_net_signal_used(rnvq);
> >               /* Both tx vq and rx socket were polled here */
> > -             mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
> > -             vhost_disable_notify(&net->dev, tvq);
> > -
> > -             preempt_disable();
> > -             endtime = busy_clock() + tvq->busyloop_timeout;
> > -
> > -             while (vhost_can_busy_poll(endtime)) {
> > -                     if (vhost_has_work(&net->dev)) {
> > -                             *busyloop_intr = true;
> > -                             break;
> > -                     }
> > -                     if ((sk_has_rx_data(sk) &&
> > -                          !vhost_vq_avail_empty(&net->dev, rvq)) ||
> > -                         !vhost_vq_avail_empty(&net->dev, tvq))
> > -                             break;
> > -                     cpu_relax();
> > -             }
> > -
> > -             preempt_enable();
> > -
> > -             if (!vhost_vq_avail_empty(&net->dev, tvq)) {
> > -                     vhost_poll_queue(&tvq->poll);
> > -             } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
> > -                     vhost_disable_notify(&net->dev, tvq);
> > -                     vhost_poll_queue(&tvq->poll);
> > -             }
> > -
> > -             mutex_unlock(&tvq->mutex);
> > +             vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
> >
> >               len = peek_head_len(rnvq, sk);
> >       }
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* [PATCH] rxrpc: Fix user ID check in rxrpc_service_prealloc_one
From: YueHaibing @ 2018-08-01  9:46 UTC (permalink / raw)
  To: davem, dhowells; +Cc: linux-kernel, netdev, linux-afs, YueHaibing

There just check the user ID isn't already in use, hence should 
compare user_call_ID with xcall->user_call_ID, which is current
node's user_call_ID.

Fixes: 540b1c48c37a ("rxrpc: Fix deadlock between call creation and sendmsg/recvmsg")
Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/rxrpc/call_accept.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index a9a9be5..9d1e298 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -116,9 +116,9 @@ static int rxrpc_service_prealloc_one(struct rxrpc_sock *rx,
 		while (*pp) {
 			parent = *pp;
 			xcall = rb_entry(parent, struct rxrpc_call, sock_node);
-			if (user_call_ID < call->user_call_ID)
+			if (user_call_ID < xcall->user_call_ID)
 				pp = &(*pp)->rb_left;
-			else if (user_call_ID > call->user_call_ID)
+			else if (user_call_ID > xcall->user_call_ID)
 				pp = &(*pp)->rb_right;
 			else
 				goto id_in_use;
-- 
2.7.0

^ permalink raw reply related

* Re: [PATCH rdma-next 08/12] overflow.h: Add arithmetic shift helper
From: Peter Zijlstra @ 2018-08-01  9:36 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Rasmus Villemoes, Leon Romanovsky, Doug Ledford, Kees Cook,
	Leon Romanovsky, RDMA mailing list, Hadar Hen Zion, Matan Barak,
	Michael J Ruhl, Noa Osherovich, Raed Salem, Yishai Hadas,
	Saeed Mahameed, linux-netdev, linux-kernel
In-Reply-To: <20180626175435.GQ5356@mellanox.com>

On Tue, Jun 26, 2018 at 11:54:35AM -0600, Jason Gunthorpe wrote:

> What about more like this?
> 
>           check_shift_overflow(a, s, d) ({

Should that not be: check_shl_overflow() ? Just 'shift' lacks a
direction.

> 	      // Shift is always performed on the machine's largest unsigned
>               u64 _a = a;
> 	      typeof(s) _s = s;
>               typeof(d) _d = d;
> 
> 	      // Make s safe against UB
> 	      unsigned int _to_shift = _s >= 0 && _s < 8*sizeof(*d) : _s ? 0;

Should we not do a gcc-plugin or something that fixes that particular
UB? Shift acting all retarded like that is just annoying. I feel we
should eliminate UBs from the language where possible, like
-fno-strict-overflow mandates 2s complement.

>               *_d = (_a << _to_shift);
> 
> 	       // s is malformed
>               (_to_shift != _s ||

Not strictly an overflow though, just not expected behaviour.

> 	       // d is a signed type and became negative
> 	       *_d < 0 ||

Only a problem if it wasn't negative to start out with.

> 	       // a is a signed type and was negative
> 	       _a < 0 ||

Why would that be a problem? You can shift left negative values just
fine. The only problem is when you run out of sign bits.

> 	       // Not invertable means a was truncated during shifting
> 	       (*_d >> _to_shift) != a))
>           })

And I'm not exactly seeing the use case for this macro. What's the point
of a shift-left if you cannot truncate bits. I suppose it's in the name
_overflow, but still.

^ permalink raw reply

* Re: [PATCH net-next] rxrpc: remove redundant variables 'xcall','sp' and 'did_discard'
From: YueHaibing @ 2018-08-01  9:19 UTC (permalink / raw)
  To: David Howells; +Cc: davem, linux-kernel, netdev, linux-afs
In-Reply-To: <15081.1533112094@warthog.procyon.org.uk>


On 2018/8/1 16:28, David Howells wrote:
> YueHaibing <yuehaibing@huawei.com> wrote:
> 
>>  		while (*pp) {
>>  			parent = *pp;
>> -			xcall = rb_entry(parent, struct rxrpc_call, sock_node);
>>  			if (user_call_ID < call->user_call_ID)
>>  				pp = &(*pp)->rb_left;
>>  			else if (user_call_ID > call->user_call_ID)
> 
> No, this is an actual bug.  The if-conditions should be using xcall-> not
> call->.

yes, I will post a new patch, thanks.

> 
>> -		sp = rxrpc_skb(skb);
> 
> Yeah, that's fine.
> 
>> -	did_discard = true;
> 
> Hmmm...  It looks like I intended something with this, but I don't remember
> what now.  I think it can be removed.
> 
> David
> 
> .
> 

^ permalink raw reply

* Re: unregister_netdevice: waiting for DEV to become free
From: Dmitry Vyukov @ 2018-08-01  9:14 UTC (permalink / raw)
  To: Steffen Klassert
  Cc: Tetsuo Handa, Herbert Xu, David S. Miller, syzbot, Dan Streetman,
	LKML, netdev, syzkaller-bugs
In-Reply-To: <20180801085749.3nf5hbxjxoafangb@gauss3.secunet.de>

On Wed, Aug 1, 2018 at 10:57 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Tue, Jul 31, 2018 at 01:31:52PM +0200, Steffen Klassert wrote:
>> On Tue, Jul 31, 2018 at 08:16:22PM +0900, Tetsuo Handa wrote:
>> > Steffen and Herbert,
>> >
>> > Do you have any question? I think I provided enough information for debugging.
>>
>> It seems that I was not on Cc at the beginning of the threat,
>> so I had to search the web for some context.
>>
>> I'm currently trying your reproducer (still with my .config) but I don't
>> hit the problem.
>
> Actually, I did not hit it because it is already fixed in my tree.
>
> It is fixed by:
>
> commit 8cc88773855f988d6a3bbf102bbd9dd9c828eb81
> xfrm: fix missing dst_release() after policy blocking lbcast and multicast
>
> This fix went to mainline on Monday.

Thanks, let's tell syzbot:

#syz fix:
xfrm: fix missing dst_release() after policy blocking lbcast and multicast

^ permalink raw reply

* Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation)
From: Dmitry Vyukov @ 2018-08-01  9:10 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Christoph Lameter, Andrey Ryabinin, Theodore Ts'o, Jan Kara,
	linux-ext4, Greg Kroah-Hartman, Pablo Neira Ayuso,
	Jozsef Kadlecsik, Florian Westphal, David Miller, NetFilter,
	coreteam, Network Development, Gerrit Renker, dccp, Jani Nikula,
	Joonas Lahtinen, Rodrigo Vivi, Dave Airlie, intel-gfx
In-Reply-To: <CACT4Y+aYZumcc-Od5T1AnP4mwn8-FaWfxvfb93MnNwQPqG8TDw@mail.gmail.com>

On Wed, Aug 1, 2018 at 10:46 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Jul 31, 2018 at 8:51 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Tue, Jul 31, 2018 at 10:49 AM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>>
>> In particular, the pattern that nf_conntrack uses looks like it is safe.
>>
>> If you have a well-defined refcount, and use "atomic_inc_not_zero()"
>> to guard the speculative RCU access section, and use
>> "atomic_dec_and_test()" in the freeing section, then you should be
>> safe wrt new allocations.
>>
>> If you have a completely new allocation that has "random stale
>> content", you know that it cannot be on the RCU list, so there is no
>> speculative access that can ever see that random content.
>>
>> So the only case you need to worry about is a re-use allocation, and
>> you know that the refcount will start out as zero even if you don't
>> have a constructor.
>>
>> So you can think of the refcount itself as always having a zero
>> constructor, *BUT* you need to be careful with ordering.
>>
>> In particular, whoever does the allocation needs to then set the
>> refcount to a non-zero value *after* it has initialized all the other
>> fields. And in particular, it needs to make sure that it uses the
>> proper memory ordering to do so.
>>
>> And in this case, we have
>>
>>   static struct nf_conn *
>>   __nf_conntrack_alloc(struct net *net,
>>   {
>>         ...
>>         atomic_set(&ct->ct_general.use, 0);
>>
>> which is a no-op for the re-use case (whether racing or not, since any
>> "inc_not_zero" users won't touch it), but initializes it to zero for
>> the "completely new object" case.
>>
>> And then, the thing that actually exposes it to the speculative walkers does:
>>
>>   int
>>   nf_conntrack_hash_check_insert(struct nf_conn *ct)
>>   {
>>         ...
>>         smp_wmb();
>>         /* The caller holds a reference to this object */
>>         atomic_set(&ct->ct_general.use, 2);
>>
>> which means that it stays as zero until everything is actually set up,
>> and then the optimistic walker can use the other fields (including
>> spinlocks etc) to verify that it's actually the right thing. The
>> smp_wmb() means that the previous initialization really will be
>> visible before the object is visible.
>>
>> Side note: on some architectures it might help to make that "smp_wmb
>> -> atomic_set()" sequence be am "smp_store_release()" instead. Doesn't
>> matter on x86, but might matter on arm64.
>>
>> NOTE! One thing to be very worried about is that re-initializing
>> whatever RCU lists means that now the RCU walker may be walking on the
>> wrong list so the walker may do the right thing for this particular
>> entry, but it may miss walking *other* entries. So then you can get
>> spurious lookup failures, because the RCU walker never walked all the
>> way to the end of the right list. That ends up being a much more
>> subtle bug.
>>
>> But the nf_conntrack case seems to get that right too, see the restart
>> in ____nf_conntrack_find().
>>
>> So I don't see anything wrong in nf_conntrack.
>>
>> But yes, using SLAB_TYPESAFE_BY_RCU is very very subtle. But most of
>> the subtleties have nothing to do with having a constructor, they are
>> about those "make sure memory ordering wrt refcount is right" and
>> "restart speculative RCU walk" issues that actually happen regardless
>> of having a constructor or not.
>
>
> Thank you, this is very enlightening.
>
> So the type-stable invariant is established by initialization of the
> object after the first kmem_cache_alloc, and then we rely on the fact
> that repeated initialization does not break the invariant, which works
> because the state is very simple (including debug builds, i.e. no
> ODEBUG nor magic values).


Still can't grasp all details.
There is state that we read without taking ct->ct_general.use ref
first, namely ct->state and what's used by nf_ct_key_equal.
So let's say the entry we want to find is in the list, but
____nf_conntrack_find finds a wrong entry earlier because all state it
looks at is random garbage, so it returns the wrong entry to
__nf_conntrack_find_get.
Now (nf_ct_is_dying(ct) || !atomic_inc_not_zero(&ct->ct_general.use))
check in __nf_conntrack_find_get passes, and it returns NULL to the
caller (which means entry is not present). While in reality the entry
is present, but we were just looking at the wrong one.

Also I am not sure about order of checks in (nf_ct_is_dying(ct) ||
!atomic_inc_not_zero(&ct->ct_general.use)), because checking state
before taking the ref is only a best-effort hint, so it can actually
be a dying entry when we take a ref.

So shouldn't it read something like the following?

        rcu_read_lock();
begin:
        h = ____nf_conntrack_find(net, zone, tuple, hash);
        if (h) {
                ct = nf_ct_tuplehash_to_ctrack(h);
                if (!atomic_inc_not_zero(&ct->ct_general.use))
                        goto begin;
                if (unlikely(nf_ct_is_dying(ct)) ||
                    unlikely(!nf_ct_key_equal(h, tuple, zone, net))) {
                        nf_ct_put(ct);
                        goto begin;
                }
        }
        rcu_read_unlock();
        return h;

^ permalink raw reply

* Re: SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation)
From: Andrey Ryabinin @ 2018-08-01  9:03 UTC (permalink / raw)
  To: Linus Torvalds, Christoph Lameter
  Cc: Dave Airlie, DRI, linux-mm, Eric Dumazet, Network Development,
	gerrit, dccp, coreteam, Jozsef Kadlecsik, Alexey Kuznetsov,
	linux-ext4, Pablo Neira Ayuso, linux-s390, Andrey Konovalov,
	intel-gfx, Ursula Braun, Rodrigo Vivi, Dmitry Vyukov,
	Theodore Ts'o, Hideaki YOSHIFUJI, Greg Kroah-Hartman,
	Florian Westphal, Linux Kernel Mailing List, NetFilter <netfi
In-Reply-To: <CA+55aFzYLgyNp1jsqsvUOjwZdO_1Piqj=iB=rzDShjScdNtkbg@mail.gmail.com>



On 07/31/2018 09:51 PM, Linus Torvalds wrote:
> On Tue, Jul 31, 2018 at 10:49 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
> 
> In particular, the pattern that nf_conntrack uses looks like it is safe.
> 
> If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> to guard the speculative RCU access section, and use
> "atomic_dec_and_test()" in the freeing section, then you should be
> safe wrt new allocations.
> 
> If you have a completely new allocation that has "random stale
> content", you know that it cannot be on the RCU list, so there is no
> speculative access that can ever see that random content.
> 
> So the only case you need to worry about is a re-use allocation, and
> you know that the refcount will start out as zero even if you don't
> have a constructor.
> 
> So you can think of the refcount itself as always having a zero
> constructor, *BUT* you need to be careful with ordering.
> 
> In particular, whoever does the allocation needs to then set the
> refcount to a non-zero value *after* it has initialized all the other
> fields. And in particular, it needs to make sure that it uses the
> proper memory ordering to do so.
> 
> And in this case, we have
> 
>   static struct nf_conn *
>   __nf_conntrack_alloc(struct net *net,
>   {
>         ...
>         atomic_set(&ct->ct_general.use, 0);
> 
> which is a no-op for the re-use case (whether racing or not, since any
> "inc_not_zero" users won't touch it), but initializes it to zero for
> the "completely new object" case.
> 
> And then, the thing that actually exposes it to the speculative walkers does:
> 
>   int
>   nf_conntrack_hash_check_insert(struct nf_conn *ct)
>   {
>         ...
>         smp_wmb();
>         /* The caller holds a reference to this object */
>         atomic_set(&ct->ct_general.use, 2);
> 
> which means that it stays as zero until everything is actually set up,
> and then the optimistic walker can use the other fields (including
> spinlocks etc) to verify that it's actually the right thing. The
> smp_wmb() means that the previous initialization really will be
> visible before the object is visible.
> 
> Side note: on some architectures it might help to make that "smp_wmb
> -> atomic_set()" sequence be am "smp_store_release()" instead. Doesn't
> matter on x86, but might matter on arm64.
> 
> NOTE! One thing to be very worried about is that re-initializing
> whatever RCU lists means that now the RCU walker may be walking on the
> wrong list so the walker may do the right thing for this particular
> entry, but it may miss walking *other* entries. So then you can get
> spurious lookup failures, because the RCU walker never walked all the
> way to the end of the right list. That ends up being a much more
> subtle bug.
> 
> But the nf_conntrack case seems to get that right too, see the restart
> in ____nf_conntrack_find().
> 
> So I don't see anything wrong in nf_conntrack.
> 
> But yes, using SLAB_TYPESAFE_BY_RCU is very very subtle. But most of
> the subtleties have nothing to do with having a constructor, they are
> about those "make sure memory ordering wrt refcount is right" and
> "restart speculative RCU walk" issues that actually happen regardless
> of having a constructor or not.
> 

I see, thanks. I just don't see any point or advantage of *not* using the constructor
with SLAB_TYPESAFE_BY_RCU caches.
There is always must be a small part of initialization code that could be placed
in constructor. And it's better to put that part into constructor to avoid initializing
what's already has been initialized. If people use SLAB_TYPESAFE_BY_RCU instead of traditional
kfree_rcu() for cache-efficiency and because they want to reuse the object faster, than avoiding
few writes into cache-hot data doesn't look like a bad idea.
E.g. nf_conntrack case could at least move the spin_lock_init() and atomic_set(&ct->ct_general.use, 0)
in the constructor.

I can't think of any advantage in not having the constructor. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply

* Re: unregister_netdevice: waiting for DEV to become free
From: Steffen Klassert @ 2018-08-01  8:57 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Herbert Xu, David S. Miller, syzbot, ddstreet, dvyukov,
	linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <20180731113152.4pes5xwhae2xydqg@gauss3.secunet.de>

On Tue, Jul 31, 2018 at 01:31:52PM +0200, Steffen Klassert wrote:
> On Tue, Jul 31, 2018 at 08:16:22PM +0900, Tetsuo Handa wrote:
> > Steffen and Herbert,
> > 
> > Do you have any question? I think I provided enough information for debugging.
> 
> It seems that I was not on Cc at the beginning of the threat,
> so I had to search the web for some context.
> 
> I'm currently trying your reproducer (still with my .config) but I don't
> hit the problem.

Actually, I did not hit it because it is already fixed in my tree.

It is fixed by:

commit 8cc88773855f988d6a3bbf102bbd9dd9c828eb81
xfrm: fix missing dst_release() after policy blocking lbcast and multicast

This fix went to mainline on Monday.

^ permalink raw reply

* Re: KASAN: use-after-free Read in vhost_transport_send_pkt
From: Dmitry Vyukov @ 2018-08-01  8:30 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: syzbot, Jason Wang, KVM list, LKML, Michael S. Tsirkin, netdev,
	Stefan Hajnoczi, syzkaller-bugs, virtualization
In-Reply-To: <20180731154351.GF17816@stefanha-x1.localdomain>

On Tue, Jul 31, 2018 at 5:43 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 30, 2018 at 11:15:03AM -0700, syzbot wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit:    acb1872577b3 Linux 4.18-rc7
>> git tree:       upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=14eb932c400000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
>> dashboard link: https://syzkaller.appspot.com/bug?extid=bd391451452fb0b93039
>> compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+bd391451452fb0b93039@syzkaller.appspotmail.com
>>
>> netlink: 'syz-executor5': attribute type 2 has an invalid length.
>> binder: 28577:28588 transaction failed 29189/-22, size 0-0 line 2852
>> ==================================================================
>> BUG: KASAN: use-after-free in debug_spin_lock_before
>> kernel/locking/spinlock_debug.c:83 [inline]
>> BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c0/0x200
>> kernel/locking/spinlock_debug.c:112
>> Read of size 4 at addr ffff880194d0ec6c by task syz-executor4/28583
>>
>> CPU: 1 PID: 28583 Comm: syz-executor4 Not tainted 4.18.0-rc7+ #169
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> Call Trace:
>>  __dump_stack lib/dump_stack.c:77 [inline]
>>  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
>>  print_address_description+0x6c/0x20b mm/kasan/report.c:256
>>  kasan_report_error mm/kasan/report.c:354 [inline]
>>  kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
>>  __asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
>>  debug_spin_lock_before kernel/locking/spinlock_debug.c:83 [inline]
>>  do_raw_spin_lock+0x1c0/0x200 kernel/locking/spinlock_debug.c:112
>>  __raw_spin_lock_bh include/linux/spinlock_api_smp.h:136 [inline]
>>  _raw_spin_lock_bh+0x39/0x40 kernel/locking/spinlock.c:168
>>  spin_lock_bh include/linux/spinlock.h:315 [inline]
>>  vhost_transport_send_pkt+0x12e/0x380 drivers/vhost/vsock.c:223
>
> Thanks for the vsock fuzzing.  This is a useful bug report.

Thanks. But note that syzkaller descriptions for vsock were written by
people who have no idea what is vsock whatsoever:
https://github.com/google/syzkaller/blob/master/sys/linux/socket_vnet.txt
So most likely fuzzing is inefficient and does not test the most
interesting parts of vsock.


> It looks like vhost_vsock_get() needs to involve a reference count so
> that vhost_vsock instances cannot be freed while something is still
> using them.
>
> The reproducer probably involves racing close() with connect().
>
> I am looking into a fix.
>
> Stefan
>
>>  virtio_transport_send_pkt_info+0x31d/0x460
>> net/vmw_vsock/virtio_transport_common.c:190
>>  virtio_transport_connect+0x17c/0x220
>> net/vmw_vsock/virtio_transport_common.c:588
>>  vsock_stream_connect+0x4fb/0xfc0 net/vmw_vsock/af_vsock.c:1197
>>  __sys_connect+0x37d/0x4c0 net/socket.c:1673
>>  __do_sys_connect net/socket.c:1684 [inline]
>>  __se_sys_connect net/socket.c:1681 [inline]
>>  __x64_sys_connect+0x73/0xb0 net/socket.c:1681
>>  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x456a09
>> Code: fd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
>> 0f 83 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007fa4aee5bc78 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
>> RAX: ffffffffffffffda RBX: 00007fa4aee5c6d4 RCX: 0000000000456a09
>> RDX: 0000000000000010 RSI: 0000000020000200 RDI: 0000000000000016
>> RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
>> R13: 00000000004ca838 R14: 00000000004c25fb R15: 0000000000000000
>>
>> Allocated by task 28583:
>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>>  set_track mm/kasan/kasan.c:460 [inline]
>>  kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
>>  __do_kmalloc_node mm/slab.c:3682 [inline]
>>  __kmalloc_node+0x47/0x70 mm/slab.c:3689
>>  kmalloc_node include/linux/slab.h:555 [inline]
>>  kvmalloc_node+0xb9/0xf0 mm/util.c:423
>>  kvmalloc include/linux/mm.h:573 [inline]
>>  vhost_vsock_dev_open+0xa2/0x5a0 drivers/vhost/vsock.c:511
>>  misc_open+0x3ca/0x560 drivers/char/misc.c:141
>>  chrdev_open+0x25a/0x770 fs/char_dev.c:417
>>  do_dentry_open+0x818/0xe40 fs/open.c:794
>>  vfs_open+0x139/0x230 fs/open.c:908
>>  do_last fs/namei.c:3399 [inline]
>>  path_openat+0x174a/0x4e10 fs/namei.c:3540
>>  do_filp_open+0x255/0x380 fs/namei.c:3574
>>  do_sys_open+0x584/0x760 fs/open.c:1101
>>  __do_sys_openat fs/open.c:1128 [inline]
>>  __se_sys_openat fs/open.c:1122 [inline]
>>  __x64_sys_openat+0x9d/0x100 fs/open.c:1122
>>  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Freed by task 28579:
>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>>  set_track mm/kasan/kasan.c:460 [inline]
>>  __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
>>  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
>>  __cache_free mm/slab.c:3498 [inline]
>>  kfree+0xd9/0x260 mm/slab.c:3813
>>  kvfree+0x61/0x70 mm/util.c:442
>>  vhost_vsock_free drivers/vhost/vsock.c:499 [inline]
>>  vhost_vsock_dev_release+0x4fd/0x750 drivers/vhost/vsock.c:604
>>  __fput+0x355/0x8b0 fs/file_table.c:209
>>  ____fput+0x15/0x20 fs/file_table.c:243
>>  task_work_run+0x1ec/0x2a0 kernel/task_work.c:113
>>  tracehook_notify_resume include/linux/tracehook.h:192 [inline]
>>  exit_to_usermode_loop+0x313/0x370 arch/x86/entry/common.c:166
>>  prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
>>  syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
>>  do_syscall_64+0x6be/0x820 arch/x86/entry/common.c:293
>>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> The buggy address belongs to the object at ffff880194d05f80
>>  which belongs to the cache kmalloc-65536 of size 65536
>> The buggy address is located 36076 bytes inside of
>>  65536-byte region [ffff880194d05f80, ffff880194d15f80)
>> The buggy address belongs to the page:
>> page:ffffea0006534000 count:1 mapcount:0 mapping:ffff8801dac02500 index:0x0
>> compound_mapcount: 0
>> flags: 0x2fffc0000008100(slab|head)
>> raw: 02fffc0000008100 ffffea0006599808 ffff8801dac01e48 ffff8801dac02500
>> raw: 0000000000000000 ffff880194d05f80 0000000100000001 0000000000000000
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>>  ffff880194d0eb00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>  ffff880194d0eb80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> > ffff880194d0ec00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>                                                           ^
>>  ffff880194d0ec80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>  ffff880194d0ed00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> ==================================================================
>>
>>
>> ---
>> This bug is generated by a bot. It may contain errors.
>> See https://goo.gl/tpsmEJ for more information about syzbot.
>> syzbot engineers can be reached at syzkaller@googlegroups.com.
>>
>> syzbot will keep track of this bug report. See:
>> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
>> syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20180731154351.GF17816%40stefanha-x1.localdomain.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: KASAN: use-after-free Read in rtnetlink_put_metrics
From: David Ahern @ 2018-08-01  8:28 UTC (permalink / raw)
  To: Sabrina Dubroca, Cong Wang
  Cc: Eric Dumazet, syzbot+41f9c04b50ef70c66947, christian.brauner,
	David Miller, Florian Westphal, Jiri Benc, Kirill Tkhai, LKML,
	lucien xin, Linux Kernel Network Developers, syzkaller-bugs
In-Reply-To: <20180801081537.GA31982@bistromath.localdomain>

On 8/1/18 10:15 AM, Sabrina Dubroca wrote:
> ip -net peerA route add fec0:B::/64 via fec0:A:: mtu 1300

I am on vacation right now with limited access to internet, so not able
to take a look.

In submitting the fib6_info changes I did tests like this and did not
see memleak reports (and verified the metrics refcnts with printks), so
I am surprised to see your patch in this area. Will take a look when I
can but that will not be for a couple more weeks.

^ permalink raw reply

* Re: [PATCH net-next] rxrpc: remove redundant variables 'xcall','sp' and 'did_discard'
From: David Howells @ 2018-08-01  8:28 UTC (permalink / raw)
  To: YueHaibing; +Cc: dhowells, davem, linux-kernel, netdev, linux-afs
In-Reply-To: <20180801073251.9808-1-yuehaibing@huawei.com>

YueHaibing <yuehaibing@huawei.com> wrote:

>  		while (*pp) {
>  			parent = *pp;
> -			xcall = rb_entry(parent, struct rxrpc_call, sock_node);
>  			if (user_call_ID < call->user_call_ID)
>  				pp = &(*pp)->rb_left;
>  			else if (user_call_ID > call->user_call_ID)

No, this is an actual bug.  The if-conditions should be using xcall-> not
call->.

> -		sp = rxrpc_skb(skb);

Yeah, that's fine.

> -	did_discard = true;

Hmmm...  It looks like I intended something with this, but I don't remember
what now.  I think it can be removed.

David

^ permalink raw reply

* INFO: rcu detected stall in tipc_sk_lookup
From: syzbot @ 2018-08-01  8:28 UTC (permalink / raw)
  To: davem, jon.maloy, linux-kernel, netdev, syzkaller-bugs,
	tipc-discussion, ying.xue

Hello,

syzbot found the following crash on:

HEAD commit:    527838d470e3 Merge branch 'x86-urgent-for-linus' of git://..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12726f1c400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2dc0cd7c2eefb46f
dashboard link: https://syzkaller.appspot.com/bug?extid=230075782a192b8ca880
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+230075782a192b8ca880@syzkaller.appspotmail.com

INFO: rcu_sched self-detected stall on CPU
	1-....: (104999 ticks this GP) idle=002/1/4611686018427387906  
softirq=125049/125058 fqs=26224
	 (t=105000 jiffies g=69343 c=69342 q=1302)
NMI backtrace for cpu 1
CPU: 1 PID: 29490 Comm: syz-executor5 Not tainted 4.18.0-rc7+ #170
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  <IRQ>
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
  nmi_cpu_backtrace.cold.4+0x19/0xce lib/nmi_backtrace.c:103
  nmi_trigger_cpumask_backtrace+0x151/0x192 lib/nmi_backtrace.c:62
  arch_trigger_cpumask_backtrace+0x14/0x20 arch/x86/kernel/apic/hw_nmi.c:38
  trigger_single_cpu_backtrace include/linux/nmi.h:156 [inline]
  rcu_dump_cpu_stacks+0x175/0x1c2 kernel/rcu/tree.c:1336
  print_cpu_stall kernel/rcu/tree.c:1485 [inline]
  check_cpu_stall.isra.60.cold.78+0x36c/0x5a6 kernel/rcu/tree.c:1553
  __rcu_pending kernel/rcu/tree.c:3244 [inline]
  rcu_pending kernel/rcu/tree.c:3291 [inline]
  rcu_check_callbacks+0x23f/0xcd0 kernel/rcu/tree.c:2646
  update_process_times+0x2d/0x70 kernel/time/timer.c:1636
  tick_sched_handle+0x9f/0x180 kernel/time/tick-sched.c:164
  tick_sched_timer+0x45/0x130 kernel/time/tick-sched.c:1274
  __run_hrtimer kernel/time/hrtimer.c:1398 [inline]
  __hrtimer_run_queues+0x3eb/0x10c0 kernel/time/hrtimer.c:1460
  hrtimer_interrupt+0x2f3/0x750 kernel/time/hrtimer.c:1518
  local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1028 [inline]
  smp_apic_timer_interrupt+0x165/0x730 arch/x86/kernel/apic/apic.c:1053
  apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:863
  </IRQ>
RIP: 0010:arch_local_irq_restore arch/x86/include/asm/paravirt.h:783  
[inline]
RIP: 0010:lock_acquire+0x25f/0x540 kernel/locking/lockdep.c:3927
Code: 00 00 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 6a 02 00 00 48 83 3d a8  
8a 92 06 00 0f 84 d4 01 00 00 48 8b bd 20 ff ff ff 57 9d <0f> 1f 44 00 00  
48 b8 00 00 00 00 00 fc ff df 48 01 c3 48 c7 03 00
RSP: 0018:ffff8801999a67c0 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
RAX: dffffc0000000000 RBX: 1ffff10033334cfd RCX: 0000000000000000
RDX: 1ffffffff0fe3615 RSI: 0000000000000000 RDI: 0000000000000286
RBP: ffff8801999a68b0 R08: 0000000000000008 R09: 0000000000000002
R10: ffff880192d5c908 R11: 632564d1cb2a3fed R12: ffff880192d5c080
R13: 0000000000000002 R14: 0000000000000000 R15: 0000000000000000
  rcu_lock_acquire include/linux/rcupdate.h:245 [inline]
  rcu_read_lock include/linux/rcupdate.h:631 [inline]
  net_generic include/net/netns/generic.h:44 [inline]
  tipc_sk_lookup+0x131/0xfa0 net/tipc/socket.c:2682
  tipc_nl_publ_dump+0x243/0xfa2 net/tipc/socket.c:3456
  __tipc_nl_compat_dumpit.isra.11+0x20b/0xad0 net/tipc/netlink_compat.c:192
  tipc_nl_compat_publ_dump net/tipc/netlink_compat.c:920 [inline]
  tipc_nl_compat_sk_dump+0x87c/0xca0 net/tipc/netlink_compat.c:968
  __tipc_nl_compat_dumpit.isra.11+0x337/0xad0 net/tipc/netlink_compat.c:201
  tipc_nl_compat_dumpit+0x1f4/0x440 net/tipc/netlink_compat.c:265
  tipc_nl_compat_handle net/tipc/netlink_compat.c:1142 [inline]
  tipc_nl_compat_recv+0x12b3/0x19a0 net/tipc/netlink_compat.c:1205
  genl_family_rcv_msg+0x8a3/0x1140 net/netlink/genetlink.c:601
  genl_rcv_msg+0xc6/0x168 net/netlink/genetlink.c:626
  netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2448
  genl_rcv+0x28/0x40 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0x5a0/0x760 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0xa18/0xfd0 net/netlink/af_netlink.c:1901
  sock_sendmsg_nosec net/socket.c:641 [inline]
  sock_sendmsg+0xd5/0x120 net/socket.c:651
  ___sys_sendmsg+0x7fd/0x930 net/socket.c:2125
  __sys_sendmsg+0x11d/0x290 net/socket.c:2163
  __do_sys_sendmsg net/socket.c:2172 [inline]
  __se_sys_sendmsg net/socket.c:2170 [inline]
  __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2170
  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x456a09
Code: fd b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 cb b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8665b39c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007f8665b3a6d4 RCX: 0000000000456a09
RDX: 0000000000000000 RSI: 0000000020000000 RDI: 0000000000000022
RBP: 00000000009300a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000004d3058 R14: 00000000004c7d3e R15: 0000000000000000


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.

^ permalink raw reply

* Re: [PATCH 07/10] dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing
From: Quentin Schulz @ 2018-08-01  8:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
	mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
	linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <20180730133448.GD13198@lunn.ch>

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

Hi Andrew,

On Mon, Jul 30, 2018 at 03:34:48PM +0200, Andrew Lunn wrote:
> > +Required properties:
> > +
> > +- compatible: should be "mscc,vsc7514-serdes"
> > +- #phy-cells : from the generic phy bindings, must be 3. The first number
> > +               defines the kind of Serdes (1 for SERDES1G_X, 6 for
> > +	       SERDES6G_X), the second defines the macros in the specified
> > +	       kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the
> > +	       last one defines the input port to use for a given SerDes
> > +	       macro,
> 
> It looks like there are some space vs tab issues here.
> 

Yup.

> > +
> > +Example:
> > +
> > +	serdes: serdes {
> 
> Maybe this should be serdes-mux? The SERDES itself should have some
> registers somewhere. If you ever decide to make use of phylink,
> e.g. to support SFP, you are going to need to know if the SERDES is
> up. So you might need to add the actual SERDES device, in addition to
> the mux for the SERDES.
> 

I'm not sure to follow.

To be honest, I might have mislead you. The whole configuration of the
serdes is in the hsio register address space. For now, muxing is the
only reason there is a driver for the serdes but there are other things
that can be configured (though not used yet): de/serializer, input/output
buffers, PLL, ... configuration registers for the SerDes.

So I think I should remove the mention to muxing and just say in the
cover letter and commit log it's for muxing the SerDes among other
configurations.

So I think we're good with the current driver and DT, just poor wording
on my side for the commit log/cover letter. Do you agree?

Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 07/10] dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing
From: Quentin Schulz @ 2018-08-01  8:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
	mark.rutland, davem, kishon, andrew, linux-mips, devicetree,
	linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <bcea7c75-e5a6-4533-aee0-65c893e8a422@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

Hi Florian,

On Mon, Jul 30, 2018 at 02:39:35PM -0700, Florian Fainelli wrote:
> On 07/30/2018 05:43 AM, Quentin Schulz wrote:
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt | 42 +++++++-
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt
> > new file mode 100644
> > index 0000000..25b102d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt
> > @@ -0,0 +1,42 @@
> > +Microsemi Ocelot SerDes muxing driver
> > +-------------------------------------
> > +
> > +On Microsemi Ocelot, there is a handful of registers in HSIO address
> > +space for setting up the SerDes to switch port muxing.
> > +
> > +A SerDes X can be "muxed" to work with switch port Y or Z for example.
> > +One specific SerDes can also be used as a PCIe interface.
> > +
> > +Hence, a SerDes represents an interface, be it an Ethernet or a PCIe one.
> > +
> > +There are two kinds of SerDes: SERDES1G supports 10/100Mbps in
> > +half/full-duplex and 1000Mbps in full-duplex mode while SERDES6G supports
> > +10/100Mbps in half/full-duplex and 1000/2500Mbps in full-duplex mode.
> > +
> > +Also, SERDES6G number (aka "macro") 0 is the only interface supporting
> > +QSGMII.
> > +
> > +Required properties:
> > +
> > +- compatible: should be "mscc,vsc7514-serdes"
> > +- #phy-cells : from the generic phy bindings, must be 3. The first number
> > +               defines the kind of Serdes (1 for SERDES1G_X, 6 for
> > +	       SERDES6G_X), the second defines the macros in the specified
> > +	       kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the
> > +	       last one defines the input port to use for a given SerDes
> > +	       macro,
> 
> It would probably be more natural to reverse some of this and have the
> 1st cell be the input port, while the 2nd and 3rd cell are the serdes
> kind and the serdes macro type. Same comment as Andrew, can you please
> define the 2nd and 3rd cells possible values in a header file that you
> can include from both the DTS and the driver making use of that?

OK for a define for the DeviceTree part.

You want one set of defines for the values in the 2nd cell and one other
set of defines for the 3rd cell?

I'm fine with a define for the second value (which is basically the enum
serdes_type I've defined at the beginning of the serdes driver) but I
don't see the point of defining the index of the SerDes. What would it
look like?

enum serdes_type {
	SERDES1G = 1,
	SERDES6G = 6,
}

#define SERDES1G_0	0
#define SERDES1G_1	1
#define SERDES1G_2	2
#define SERDES6G_0	0
#define SERDES6G_1	1

Then, e.g.:

&port5 {
	phys = <&serdes 5 SERDES1G SERDES1G_0>
};

If you want a define for the pair (serdes_type, serdes_index), I don't
see how I could re-use it on the driver side but it makes more sense on the
DeviceTree side:

#define SERDES1G_0	1 0
#define SERDES1G_1	1 1
#define SERDES1G_2	1 2
#define SERDES6G_0	6 0
#define SERDES6G_1	6 1

Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: KASAN: use-after-free Read in rtnetlink_put_metrics
From: Sabrina Dubroca @ 2018-08-01  8:15 UTC (permalink / raw)
  To: Cong Wang
  Cc: Eric Dumazet, syzbot+41f9c04b50ef70c66947, christian.brauner,
	David Miller, David Ahern, Florian Westphal, Jiri Benc,
	Kirill Tkhai, LKML, lucien xin, Linux Kernel Network Developers,
	syzkaller-bugs
In-Reply-To: <CAM_iQpURKOMAaPfs9v7FzhqqQPzj71sQD2LS0eU8cuqTML-YoA@mail.gmail.com>

2018-07-31, 16:03:13 -0700, Cong Wang wrote:
> On Tue, Jul 31, 2018 at 6:41 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2018-07-31, 05:41:56 -0700, Eric Dumazet wrote:
> > >
> > >
> > > On 07/31/2018 05:31 AM, syzbot wrote:
> > > > Hello,
> > > >
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit:    61f4b23769f0 netlink: Don't shift with UB on nlk->ngroups
> > > > git tree:       net
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14a9de58400000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=ffb4428fdc82f93b
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=41f9c04b50ef70c66947
> > > > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > > >
> > > > Unfortunately, I don't have any reproducer for this crash yet.
> > [...]
> >
> > > Probably also caused by :
> > >
> > >
> > > commit df18b50448fab1dff093731dfd0e25e77e1afcd1
> > > Author: Sabrina Dubroca <sd@queasysnail.net>
> > > Date:   Mon Jul 30 16:23:10 2018 +0200
> > >
> > >     net/ipv6: fix metrics leak
> >
> > Yeah, I'm looking into both those reports :/
> 
> Looks like this commit is completely unnecessary,
> fib6_drop_pcpu_from() calls fib6_info_release()
> which calls fib6_info_destroy_rcu(), so this metrics
> will be released twice...

kmemleak disagrees:

unreferenced object 0xffff88006b605080 (size 96):
  comm "ip", pid 433, jiffies 4294889793 (age 74.844s)
  hex dump (first 32 bytes):
    00 00 00 00 f4 01 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000002650e4e2>] ip6_route_info_create+0x770/0x4050
    [<000000000a8d4c52>] ip6_route_add+0x18/0x90
    [<00000000474d669c>] inet6_rtm_newroute+0xeb/0x100
    [<0000000019fb732d>] rtnetlink_rcv_msg+0x3b5/0xb40
    [<000000006f891e19>] netlink_rcv_skb+0x137/0x380
    [<0000000070451985>] netlink_unicast+0x47f/0x6e0
    [<000000004487d656>] netlink_sendmsg+0x7a7/0x10c0
    [<0000000089fdf5ae>] sock_sendmsg+0xac/0x160
    [<00000000aae19c54>] ___sys_sendmsg+0x6e0/0xbb0
    [<00000000a3906352>] __sys_sendmsg+0xdc/0x230
    [<00000000c7c8548a>] do_syscall_64+0x15d/0x740
    [<000000007dfdad73>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
    [<000000003adb705a>] 0xffffffffffffffff


ip netns add peerA
ip link add eth0 netns peerA type veth peer name ethA
ip -net peerA link set eth0 up
ip -net peerA link set lo up
ip -net peerA a a fec0:A::1/64 dev eth0
ip -net peerA route add fec0:B::/64 via fec0:A:: mtu 1300

ip netns exec peerA nc fec0:B::1 1234

ip -net peerA route del fec0:B::/64
ip netns del peerA

-- 
Sabrina

^ permalink raw reply

* Re: [PATCH 00/10] mscc: ocelot: add support for SerDes muxing configuration
From: Quentin Schulz @ 2018-08-01  7:54 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
	mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
	linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <20180730132441.GC13198@lunn.ch>

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Hi Andrew,

On Mon, Jul 30, 2018 at 03:24:41PM +0200, Andrew Lunn wrote:
> > The SerDes configuration is in the middle of an address space (HSIO) that
> > is used to configure some parts in the MAC controller driver, that is why
> > we need to use a syscon so that we can write to the same address space from
> > different drivers safely using regmap.
> 
> Hi Quentin
> 
> I assume breaking backwards compatibility is not an issue here, since
> there currently is only one board using the DT binding. But it would
> be good to give a warning in the cover notes. git bisect will also
> break for this one particular board. And since these changes are going
> through different trees, it could be quite a big break.
> 

Yes sorry, I should have mentioned it in the cover letter, will do
if/when there is a v2.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH net-next 10/10] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration
From: Quentin Schulz @ 2018-08-01  7:51 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: alexandre.belloni, ralf, paul.burton, jhogan, robh+dt,
	mark.rutland, davem, kishon, f.fainelli, linux-mips, devicetree,
	linux-kernel, netdev, allan.nielsen, thomas.petazzoni
In-Reply-To: <20180730135018.GF13198@lunn.ch>

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

Hi Andrew,

On Mon, Jul 30, 2018 at 03:50:18PM +0200, Andrew Lunn wrote:
>  On Mon, Jul 30, 2018 at 02:43:55PM +0200, Quentin Schulz wrote:
> 
> > +		err = of_get_phy_mode(portnp);
> > +		if (err < 0)
> > +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> > +		else
> > +			ocelot->ports[port]->phy_mode = err;
> > +
> > +		if (ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_NA)
> > +			continue;
> > +
> > +		if (ocelot->ports[port]->phy_mode == PHY_INTERFACE_MODE_SGMII)
> > +			phy_mode = PHY_MODE_SGMII;
> > +		else
> > +			phy_mode = PHY_MODE_QSGMII;
> 
> Hi Quentin
> 
> Say somebody puts RGMII as the phy-mode? It would be better to verify
> it is only SGMII or QSGMII and return -EINVAL otherwise.
> 

I'll replace this with a switch case to handle other cases.

> > +
> > +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> > +		if (IS_ERR(serdes)) {
> > +			if (PTR_ERR(serdes) == -EPROBE_DEFER) {
> > +				dev_err(ocelot->dev, "deferring probe\n");
> 
> dev_dbg() ? It is not really an error.
> 

Ack.

> > +				err = -EPROBE_DEFER;
> > +				goto err_probe_ports;
> > +			}
> > +
> > +			dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
> > +				port);
> > +			err = -ENODEV;
> 
> err = PTR_ERR(serdes) so we get the actual error?
> 

Ack.

Thanks,
Quentin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [PATCH net-next] rxrpc: remove redundant variables 'xcall','sp' and 'did_discard'
From: YueHaibing @ 2018-08-01  7:32 UTC (permalink / raw)
  To: davem, dhowells; +Cc: linux-kernel, netdev, linux-afs, YueHaibing

Variables 'xcall','sp' and 'did_discard' are being assigned,
but are never used,hence they are redundant and can be removed.

fix fllowing warning:

net/rxrpc/call_accept.c:110:22: warning: variable ‘xcall’ set but not used [-Wunused-but-set-variable]
net/rxrpc/call_event.c:165:25: warning: variable ‘sp’ set but not used [-Wunused-but-set-variable]
net/rxrpc/conn_client.c:1054:7: warning: variable ‘did_discard’ set but not used [-Wunused-but-set-variable]

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 net/rxrpc/call_accept.c | 2 --
 net/rxrpc/call_event.c  | 2 --
 net/rxrpc/conn_client.c | 2 --
 3 files changed, 6 deletions(-)

diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index a9a9be5..7487a62 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -107,7 +107,6 @@ static int rxrpc_service_prealloc_one(struct rxrpc_sock *rx,
 
 	write_lock(&rx->call_lock);
 	if (user_attach_call) {
-		struct rxrpc_call *xcall;
 		struct rb_node *parent, **pp;
 
 		/* Check the user ID isn't already in use */
@@ -115,7 +114,6 @@ static int rxrpc_service_prealloc_one(struct rxrpc_sock *rx,
 		parent = NULL;
 		while (*pp) {
 			parent = *pp;
-			xcall = rb_entry(parent, struct rxrpc_call, sock_node);
 			if (user_call_ID < call->user_call_ID)
 				pp = &(*pp)->rb_left;
 			else if (user_call_ID > call->user_call_ID)
diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index 2021041..8e7434e 100644
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -162,7 +162,6 @@ static void rxrpc_congestion_timeout(struct rxrpc_call *call)
  */
 static void rxrpc_resend(struct rxrpc_call *call, unsigned long now_j)
 {
-	struct rxrpc_skb_priv *sp;
 	struct sk_buff *skb;
 	unsigned long resend_at;
 	rxrpc_seq_t cursor, seq, top;
@@ -207,7 +206,6 @@ static void rxrpc_resend(struct rxrpc_call *call, unsigned long now_j)
 
 		skb = call->rxtx_buffer[ix];
 		rxrpc_see_skb(skb, rxrpc_skb_tx_seen);
-		sp = rxrpc_skb(skb);
 
 		if (anno_type == RXRPC_TX_ANNO_UNACK) {
 			if (ktime_after(skb->tstamp, max_age)) {
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 5736f64..e4bfbd7 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -1051,7 +1051,6 @@ void rxrpc_discard_expired_client_conns(struct work_struct *work)
 		container_of(work, struct rxrpc_net, client_conn_reaper);
 	unsigned long expiry, conn_expires_at, now;
 	unsigned int nr_conns;
-	bool did_discard = false;
 
 	_enter("");
 
@@ -1113,7 +1112,6 @@ void rxrpc_discard_expired_client_conns(struct work_struct *work)
 	 * If someone re-sets the flag and re-gets the ref, that's fine.
 	 */
 	rxrpc_put_connection(conn);
-	did_discard = true;
 	nr_conns--;
 	goto next;
 
-- 
2.7.0

^ permalink raw reply related

* [E1000-devel] [PATCH] igb: CPU0 latency and updating statistics
From: JABLONSKY Jan @ 2018-08-01  7:29 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: e1000-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org

The Watchdog workqueue in igb driver is scheduled every 2s for each 
network interface. That includes updating a statistics protected by 
spinlock. Function igb_update_stats in this case will be protected 
against preemption. According to number of a statistics registers 
(cca 60), processing this function might cause additional cpu load
 on CPU0.

In case of statistics spinlock may be replaced with mutex, which 
reduce latency on CPU0.


# Measurements
Measurements were performed on Intel Atom E3950 (Quad-Core), 
E3930 (Dual-Core)

# Cyclictest
High latency is always on CPU0 which might be in range 
from 70us to 140us (sometimes higher)


# cyclictest -l1000000 -Smp90 -i200 -q

# E3930
# default
T: 0 ( 1745) P:90 I:200 C:1000000 Min: 2 Act: 3 Avg: 3 Max: 72
T: 1 ( 1746) P:90 I:700 C: 285737 Min: 3 Act: 4 Avg: 3 Max: 13

# with patch
T: 0 (  932) P:90 I:200 C:1000000 Min: 2 Act: 3 Avg: 3 Max: 22
T: 1 (  933) P:90 I:700 C: 285736 Min: 2 Act: 3 Avg: 3 Max: 12


# E3950
# default
T: 0 ( 1103) P:90 I:200 C:1000000 Min: 2 Act: 2 Avg: 2 Max: 89
T: 1 ( 1104) P:90 I:700 C: 285738 Min: 2 Act: 4 Avg: 2 Max: 9
T: 2 ( 1105) P:90 I:1200 C: 166678 Min: 2 Act: 5 Avg: 3 Max: 22
T: 3 ( 1106) P:90 I:1700 C: 117653 Min: 2 Act: 6 Avg: 2 Max: 13

# with patch
T: 0 (  879) P:90 I:200 C:1000000 Min: 2 Act: 3 Avg: 3 Max: 26
T: 1 (  880) P:90 I:700 C: 285729 Min: 3 Act: 4 Avg: 3 Max: 22
T: 2 (  881) P:90 I:1200 C: 166672 Min: 3 Act: 7 Avg: 3 Max: 24
T: 3 (  882) P:90 I:1700 C: 117649 Min: 3 Act: 7 Avg: 3 Max: 8


Additionally I think updating statistics and watchdog task should 
be implemented in 2 separated ways

Patch is created against a linux-4.4

Reviewed also by: Bernhard Kaindl  <bernhard.kaindl@thalesgroup.com>

Signed-off-by: Jan Jablonsky <jan.jablonsky@thalesgroup.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |  2 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c    | 14 +++++++-------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h
b/drivers/net/ethernet/intel/igb/igb.h
index e3cb93b..85d68ea 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -401,7 +401,7 @@ struct igb_adapter {
 	/* OS defined structs */
 	struct pci_dev *pdev;
 
-	spinlock_t stats64_lock;
+	struct mutex stats64_lock;
 	struct rtnl_link_stats64 stats64;
 
 	/* structs defined in e1000_hw.h */
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 2529bc6..07cc2b6 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2276,7 +2276,7 @@ static void igb_get_ethtool_stats(struct
net_device *netdev,
 	int i, j;
 	char *p;
 
-	spin_lock(&adapter->stats64_lock);
+	mutex_lock(&adapter->stats64_lock);
 	igb_update_stats(adapter, net_stats);
 
 	for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++) {
@@ -2319,7 +2319,7 @@ static void igb_get_ethtool_stats(struct
net_device *netdev,
 		} while (u64_stats_fetch_retry_irq(&ring->rx_syncp, start));
 		i += IGB_RX_QUEUE_STATS_LEN;
 	}
-	spin_unlock(&adapter->stats64_lock);
+	mutex_unlock(&adapter->stats64_lock);
 }
 
 static void igb_get_strings(struct net_device *netdev, u32 stringset,
u8 *data)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index 02b23f6..9d22398 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1810,9 +1810,9 @@ void igb_down(struct igb_adapter *adapter)
 	del_timer_sync(&adapter->phy_info_timer);
 
 	/* record the stats before reset*/
-	spin_lock(&adapter->stats64_lock);
+	mutex_lock(&adapter->stats64_lock);
 	igb_update_stats(adapter, &adapter->stats64);
-	spin_unlock(&adapter->stats64_lock);
+	mutex_unlock(&adapter->stats64_lock);
 
 	adapter->link_speed = 0;
 	adapter->link_duplex = 0;
@@ -2975,7 +2975,7 @@ static int igb_sw_init(struct igb_adapter
*adapter)
 				  VLAN_HLEN;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
-	spin_lock_init(&adapter->stats64_lock);
+	mutex_init(&adapter->stats64_lock);
 #ifdef CONFIG_PCI_IOV
 	switch (hw->mac.type) {
 	case e1000_82576:
@@ -4367,9 +4367,9 @@ static void igb_watchdog_task(struct work_struct
*work)
 		}
 	}
 
-	spin_lock(&adapter->stats64_lock);
+	mutex_lock(&adapter->stats64_lock);
 	igb_update_stats(adapter, &adapter->stats64);
-	spin_unlock(&adapter->stats64_lock);
+	mutex_unlock(&adapter->stats64_lock);
 
 	for (i = 0; i < adapter->num_tx_queues; i++) {
 		struct igb_ring *tx_ring = adapter->tx_ring[i];
@@ -5152,10 +5152,10 @@ static struct rtnl_link_stats64
*igb_get_stats64(struct net_device *netdev,
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 
-	spin_lock(&adapter->stats64_lock);
+	mutex_lock(&adapter->stats64_lock);
 	igb_update_stats(adapter, &adapter->stats64);
 	memcpy(stats, &adapter->stats64, sizeof(*stats));
-	spin_unlock(&adapter->stats64_lock);
+	mutex_unlock(&adapter->stats64_lock);
 
 	return stats;
 }

^ permalink raw reply related

* Re: [PATCH v6 bpf-next 4/9] veth: Handle xdp_frames in xdp napi ring
From: Toshiaki Makita @ 2018-08-01  5:41 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Jakub Kicinski,
	John Fastabend, Karlsson, Magnus, Björn Töpel
In-Reply-To: <20180731144646.70e2171d@redhat.com>

On 2018/07/31 21:46, Jesper Dangaard Brouer wrote:
> On Tue, 31 Jul 2018 19:40:08 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
>> On 2018/07/31 19:26, Jesper Dangaard Brouer wrote:
>>>
>>> Context needed from: [PATCH v6 bpf-next 2/9] veth: Add driver XDP
>>>
>>> On Mon, 30 Jul 2018 19:43:44 +0900
>>> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>   
>>>> +static struct sk_buff *veth_build_skb(void *head, int headroom, int len,
>>>> +				      int buflen)
>>>> +{
>>>> +	struct sk_buff *skb;
>>>> +
>>>> +	if (!buflen) {
>>>> +		buflen = SKB_DATA_ALIGN(headroom + len) +
>>>> +			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>>>> +	}
>>>> +	skb = build_skb(head, buflen);
>>>> +	if (!skb)
>>>> +		return NULL;
>>>> +
>>>> +	skb_reserve(skb, headroom);
>>>> +	skb_put(skb, len);
>>>> +
>>>> +	return skb;
>>>> +}  
>>>
>>>
>>> On Mon, 30 Jul 2018 19:43:46 +0900
>>> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>   
>>>> +static struct sk_buff *veth_xdp_rcv_one(struct veth_priv *priv,
>>>> +					struct xdp_frame *frame)
>>>> +{
>>>> +	int len = frame->len, delta = 0;
>>>> +	struct bpf_prog *xdp_prog;
>>>> +	unsigned int headroom;
>>>> +	struct sk_buff *skb;
>>>> +
>>>> +	rcu_read_lock();
>>>> +	xdp_prog = rcu_dereference(priv->xdp_prog);
>>>> +	if (likely(xdp_prog)) {
>>>> +		struct xdp_buff xdp;
>>>> +		u32 act;
>>>> +
>>>> +		xdp.data_hard_start = frame->data - frame->headroom;
>>>> +		xdp.data = frame->data;
>>>> +		xdp.data_end = frame->data + frame->len;
>>>> +		xdp.data_meta = frame->data - frame->metasize;
>>>> +		xdp.rxq = &priv->xdp_rxq;
>>>> +
>>>> +		act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>> +
>>>> +		switch (act) {
>>>> +		case XDP_PASS:
>>>> +			delta = frame->data - xdp.data;
>>>> +			len = xdp.data_end - xdp.data;
>>>> +			break;
>>>> +		default:
>>>> +			bpf_warn_invalid_xdp_action(act);
>>>> +		case XDP_ABORTED:
>>>> +			trace_xdp_exception(priv->dev, xdp_prog, act);
>>>> +		case XDP_DROP:
>>>> +			goto err_xdp;
>>>> +		}
>>>> +	}
>>>> +	rcu_read_unlock();
>>>> +
>>>> +	headroom = frame->data - delta - (void *)frame;
>>>> +	skb = veth_build_skb(frame, headroom, len, 0);  
>>>
>>> Here you are adding an assumption that struct xdp_frame is always
>>> located in-the-top of the packet-data area.  I tried hard not to add
>>> such a dependency!  You can calculate the beginning of the frame from
>>> the xdp_frame->data pointer.
>>>
>>> Why not add such a dependency?  Because for AF_XDP zero-copy, we cannot
>>> make such an assumption.  
>>>
>>> Currently, when an RX-queue is in AF-XDP-ZC mode (MEM_TYPE_ZERO_COPY)
>>> the packet will get dropped when calling convert_to_xdp_frame(), but as
>>> the TODO comment indicated in convert_to_xdp_frame() this is not the
>>> end-goal. 
>>>
>>> The comment in convert_to_xdp_frame(), indicate we need a full
>>> alloc+copy, but that is actually not necessary, if we can just use
>>> another memory area for struct xdp_frame, and a pointer to data.  Thus,
>>> allowing devmap-redir to work-ZC and allow cpumap-redir to do the copy
>>> on the remote CPU.  
>>
>> Thanks for pointing this out.
>> Seems you are saying xdp_frame area is not reusable. That means we
>> reduce usable headroom on every REDIRECT. I wanted to avoid this but
>> actually it is impossible, right?
> 
> I'm not sure I understand fully...  has this something to do, with the
> below memset?

Sorry for not being so clear...
It has something to do with the memset as well but mainly I was talking
about XDP_TX and REDIRECT introduced in patch 8. On REDIRECT,
dev_map_enqueue() calls convert_to_xdp_frame() so we use the headroom
for struct xdp_frame on REDIRECT. If we don't reuse xdp_frame region of
the original xdp packet, we reduce the headroom size each time on
REDIRECT. When ZC is used, in the future xdp_frame can be non-contiguous
to the buffer, so we cannot reuse the xdp_frame region in
convert_to_xdp_frame()? But current convert_to_xdp_frame()
implementation requires xdp_frame region in headroom so I think I cannot
avoid this dependency now.

SKB has a similar problem if we cannot reuse it. It can be passed to a
bridge and redirected to another veth which has driver XDP. In that case
we need to reallocate the page if we have reduced the headroom because
sufficient headroom is required for XDP processing for now (can we
remove this requirement actually?).
Instead I think I need to drop the packet (or reallocate the buffer and
copy data) for ZC according to this patch.
https://patchwork.codeaurora.org/patch/540887/

> When cpumap generate an SKB for the netstack, then we sacrifice/reduce
> the SKB headroom available, by in convert_to_xdp_frame() reducing the
> headroom by xdp_frame size.
> 
>  xdp_frame->headroom = headroom - sizeof(*xdp_frame)
> 
> In-order to avoid doing such memset of this area.  We are actually only
> worried about exposing the 'data' pointer, thus we could just clear
> that.  (See commit 6dfb970d3dbd, this is because Alexei is planing to
> move from CAP_SYS_ADMIN to lesser privileged mode CAP_NET_ADMIN)
> 
> See commits:
>  97e19cce05e5 ("bpf: reserve xdp_frame size in xdp headroom")
>  6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page reuse")

We have talked about that...
https://patchwork.ozlabs.org/patch/903536/

The memset is introduced as per your feedback, but I'm still not sure if
we need this. In general the headroom is not cleared after allocation in
drivers, so anyway unprivileged users should not see it no matter if it
contains xdp_frame or not...

> 
> 
>>>> +	if (!skb) {
>>>> +		xdp_return_frame(frame);
>>>> +		goto err;
>>>> +	}
>>>> +
>>>> +	memset(frame, 0, sizeof(*frame));
> 
> This memset can become a performance issue later, if we change the size
> of xdp_frame. (e.g I'm considering to extend this with the DMA addr,
> but I'm not sure about that scheme yet).
> 
> Currently sizeof(xdp_frame) == 32 bytes, and a memset of 32 bytes is
> fast, due to compiler reasons.  Above 32 bytes are more expensive,
> because the compiler translates this into a "rep stos" operation, which
> is slower, as it needs to save some registers (to allow it to be
> interrupted). See [1] for experiments.
> 
> [1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/time_bench_memset.c
> 
>>>> +	skb->protocol = eth_type_trans(skb, priv->dev);
>>>> +err:
>>>> +	return skb;
>>>> +err_xdp:
>>>> +	rcu_read_unlock();
>>>> +	xdp_return_frame(frame);
>>>> +
>>>> +	return NULL;
>>>> +}  
>>>
>>>   
>>
> 
> 
> 

-- 
Toshiaki Makita

^ permalink raw reply

* Re: [PATCH v1 2/3] zinc: Introduce minimal cryptography library
From: Eric Biggers @ 2018-08-01  7:22 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-crypto, linux-kernel, netdev, davem, Andy Lutomirski,
	Greg KH, Samuel Neves, D . J . Bernstein, Tanja Lange,
	Jean-Philippe Aumasson, Karthikeyan Bhargavan

[+Cc linux-crypto]

Hi Jason,

Apologies for starting a new thread, but this patch apparently wasn't
Cc'ed to linux-crypto, despite adding over 24000 lines of crypto code.
So much for WireGuard being only 4000 lines :-)

(For anyone else like me who didn't receive the patch, it can be found at
https://patchwork.ozlabs.org/patch/951763/)

I have some preliminary comments below.

On Tue, 31 Jul 2018 21:11:01 +0200, Jason A. Donenfeld wrote:
> [PATCH v1 2/3] zinc: Introduce minimal cryptography library 
> 
> Zinc stands for "Zinc Is Not crypto/". It's also short, easy to type,
> and plays nicely with the recent trend of naming crypto libraries after
> elements. The guiding principle is "don't overdo it". It's less of a
> library and more of a directory tree for organizing well-curated direct
> implementations of cryptography primitives.
> 
> Zinc is a new cryptography API that is much more minimal and lower-level
> than the current one. It intends to complement it and provide a basis
> upon which the current crypto API might build, and perhaps someday Zinc
> may altogether supplant the current crypto API. It is motivated by
> three primary observations in crypto API design:
> 
>   * Highly composable "cipher modes" and related abstractions from
>     90s cryptographers did not turn out to be as terrific an idea as
>     hoped, leading to a host of API misuse problems.
> 
>   * Most programmers are afraid of crypto code, and so prefer to
>     integrate it into libraries in a highly abstracted manner, so as to
>     shield themselves from implementation details. Cryptographers, on
>     the other hand, prefer simple direct implementations, which they're
>     able to verify for high assurance and optimize in accordance with
>     their expertise.
> 
>   * Overly abstracted and flexible cryptography APIs lead to a host of
>     dangerous problems and performance issues. The kernel is in the
>     business usually not of coming up with new uses of crypto, but
>     rather implementing various constructions, which means it essentially
>     needs a library of primitives, not a highly abstracted enterprise-ready
>     pluggable system, with a few particular exceptions.
> 
> This last observation has seen itself play out several times over and
> over again within the kernel:
> 
>   * The perennial move of actual primitives away from crypto/ and into
>     lib/, so that users can actually call these functions directly with
>     no overhead and without lots of allocations, function pointers,
>     string specifier parsing, and general clunkiness. For example:
>     sha256, chacha20, siphash, sha1, and so forth live in lib/ rather
>     than in crypto/. Zinc intends to stop the cluttering of lib/ and
>     introduce these direct primitives into their proper place, lib/zinc/.
> 
>   * An abundance of misuse bugs with the present crypto API that have
>     been very unpleasant to clean up.
> 
>   * A hesitance to even use cryptography, because of the overhead and
>     headaches involved in accessing the routines.
> 
> Zinc goes in a rather different direction. Rather than providing a
> thoroughly designed and abstracted API, Zinc gives you simple functions,
> which implement some primitive, or some particular and specific
> construction of primitives. It is not dynamic in the least, though one
> could imagine implementing a complex dynamic dispatch mechanism (such as
> the current crypto API) on top of these basic functions. After all,
> dynamic dispatch is usually needed for applications with cipher agility,
> such as IPsec, dm-crypt, AF_ALG, and so forth, and the existing crypto
> API will continue to play that role. However, Zinc will provide a non-
> haphazard way of directly utilizing crypto routines in applications
> that do have neither need nor desire for abstraction and dynamic
> dispatch.
> 
> It also organizes the implementations in a simple, straight-forward,
> and direct manner, making it enjoyable and intuitive to work on.
> Rather than moving optimized assembly implementations into arch/, it
> keeps them all together in lib/zinc/, making it simple and obvious to
> compare and contrast what's happening. This is, notably, exactly what
> the lib/raid6/ tree does, and that seems to work out rather well. It's
> also the pattern of most successful crypto libraries. Likewise, the
> cascade of architecture-specific functions is done as ifdefs within one
> file, so that it's easy and obvious and clear what's happening, how each
> architecture differs, and how to optimize for shared patterns. This is
> very much preferable to architecture-specific file splitting.
> 
> All implementations have been extensively tested and fuzzed, and are
> selected for their quality, trustworthiness, and performance. Wherever
> possible and performant, formally verified implementations are used,
> such as those from HACL* [1] and Fiat-Crypto [2]. The routines also take
> special care to zero out secrets using memzero_explicit (and future work
> is planned to have gcc do this more reliably and performantly with
> compiler plugins). The performance of the selected implementations is
> state-of-the-art and unrivaled. Each implementation also comes with
> extensive self-tests and crafted test vectors, pulled from various
> places such as Wycheproof [9].
> 
> Regularity of function signatures is important, so that users can easily
> "guess" the name of the function they want. Though, individual
> primitives are oftentimes not trivially interchangeable, having been
> designed for different things and requiring different parameters and
> semantics, and so the function signatures they provide will directly
> reflect the realities of the primitives' usages, rather than hiding it
> behind (inevitably leaky) abstractions. Also, in contrast to the current
> crypto API, Zinc functions can work on stack buffers, and can be called
> with different keys, without requiring allocations or locking.
> 
> SIMD is used automatically when available, though some routines may
> benefit from either having their SIMD disabled for particular
> invocations, or to have the SIMD initialization calls amortized over
> several invocations of the function, and so Zinc provides helpers and
> function signatures enabling that.
> 
> More generally, Zinc provides function signatures that allow just what
> is required by the various callers. This isn't to say that users of the
> functions will be permitted to pollute the function semantics with weird
> particular needs, but we are trying very hard not to overdo it, and that
> means looking carefully at what's actually necessary, and doing just that,
> and not much more than that. Remember: practicality and cleanliness rather
> than over-zealous infrastructure.
> 
> Zinc provides also an opening for the best implementers in academia to
> contribute their time and effort to the kernel, by being sufficiently
> simple and inviting. In discussing this commit with some of the best and
> brightest over the last few years, there are many who are eager to
> devote rare talent and energy to this effort.
> 
> This initial commit adds implementations of the primitives used by WireGuard:
> 
>   * Curve25519 [3]: formally verified 64-bit C, formally verified 32-bit C,
>     x86_64 BMI2, x86_64 ADX, ARM NEON.
> 
>   * ChaCha20 [4]: generic C, x86_64 SSSE3, x86_64 AVX-2, x86_64 AVX-512F,
>     x86_64 AVX-512VL, ARM NEON, ARM64 NEON, MIPS.
> 
>   * HChaCha20 [5]: generic C, x86_64 SSSE3.
> 
>   * Poly1305 [6]: generic C, x86_64, x86_64 AVX, x86_64 AVX-2, x86_64
>     AVX-512F, ARM NEON, ARM64 NEON, MIPS, MIPS64.
> 
>   * BLAKE2s [7]: generic C, x86_64 AVX, x86_64 AVX-512VL.
> 
>   * ChaCha20Poly1305 [8]: generic C construction for both full buffers and
>     scatter gather.
> 
>   * XChaCha20Poly1305 [5]: generic C construction.
> 
> Following the merging of this, I expect for first the primitives that
> currently exist in lib/ to work their way into lib/zinc/, after intense
> scrutiny of each implementation, potentially replacing them with either
> formally-verified implementations, or better studied and faster
> state-of-the-art implementations. In a phase after that, I envision that
> certain instances from crypto/ will want to rebase themselves to simply
> be abstracted crypto API wrappers using the lower level Zinc functions.
> This is already what various crypto/ implementations do with the
> existing code in lib/.
> 
> Currently Zinc exists as a single un-menued option, CONFIG_ZINC, but as
> this grows we will inevitably want to make that more granular. This will
> happen at the appropriate time, rather than doing so prematurely. There
> also is a CONFIG_ZINC_DEBUG menued option, performing several intense
> tests at startup and enabling various BUG_ONs.
> 
> [1] https://github.com/project-everest/hacl-star
> [2] https://github.com/mit-plv/fiat-crypto
> [3] https://cr.yp.to/ecdh.html
> [4] https://cr.yp.to/chacha.html
> [5] https://cr.yp.to/snuffle/xsalsa-20081128.pdf
> [6] https://cr.yp.to/mac.html
> [7] https://blake2.net/
> [8] https://tools.ietf.org/html/rfc8439
> [9] https://github.com/google/wycheproof
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Samuel Neves <sneves@dei.uc.pt>
> Cc: D. J. Bernstein <djb@cr.yp.to>
> Cc: Tanja Lange <tanja@hyperelliptic.org>
> Cc: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
> Cc: Karthikeyan Bhargavan <karthik.bhargavan@gmail.com>
> ---
>  MAINTAINERS                             |    7 +
>  include/zinc/blake2s.h                  |   94 +
>  include/zinc/chacha20.h                 |   46 +
>  include/zinc/chacha20poly1305.h         |   51 +
>  include/zinc/curve25519.h               |   25 +
>  include/zinc/poly1305.h                 |   34 +
>  include/zinc/simd.h                     |   60 +
>  lib/Kconfig                             |   21 +
>  lib/Makefile                            |    2 +
>  lib/zinc/Makefile                       |   28 +
>  lib/zinc/blake2s/blake2s-x86_64.S       |  685 ++++++
>  lib/zinc/blake2s/blake2s.c              |  292 +++
>  lib/zinc/chacha20/chacha20-arm.S        | 1471 ++++++++++++
>  lib/zinc/chacha20/chacha20-arm64.S      | 1940 ++++++++++++++++
>  lib/zinc/chacha20/chacha20-mips.S       |  474 ++++
>  lib/zinc/chacha20/chacha20-x86_64.S     | 2630 +++++++++++++++++++++
>  lib/zinc/chacha20/chacha20.c            |  242 ++
>  lib/zinc/chacha20poly1305.c             |  286 +++
>  lib/zinc/curve25519/curve25519-arm.S    | 2110 +++++++++++++++++
>  lib/zinc/curve25519/curve25519-arm.h    |   14 +
>  lib/zinc/curve25519/curve25519-fiat32.h |  838 +++++++
>  lib/zinc/curve25519/curve25519-hacl64.h |  751 ++++++
>  lib/zinc/curve25519/curve25519-x86_64.h | 2060 +++++++++++++++++
>  lib/zinc/curve25519/curve25519.c        |   86 +
>  lib/zinc/main.c                         |   36 +
>  lib/zinc/poly1305/poly1305-arm.S        | 1115 +++++++++
>  lib/zinc/poly1305/poly1305-arm64.S      |  820 +++++++
>  lib/zinc/poly1305/poly1305-mips.S       |  417 ++++
>  lib/zinc/poly1305/poly1305-mips64.S     |  357 +++
>  lib/zinc/poly1305/poly1305-x86_64.S     | 2790 +++++++++++++++++++++++
>  lib/zinc/poly1305/poly1305.c            |  377 +++
>  lib/zinc/selftest/blake2s.h             |  559 +++++
>  lib/zinc/selftest/chacha20poly1305.h    | 1559 +++++++++++++
>  lib/zinc/selftest/curve25519.h          |  607 +++++
>  lib/zinc/selftest/poly1305.h            | 1568 +++++++++++++
>  35 files changed, 24452 insertions(+)
>  create mode 100644 include/zinc/blake2s.h
>  create mode 100644 include/zinc/chacha20.h
>  create mode 100644 include/zinc/chacha20poly1305.h
>  create mode 100644 include/zinc/curve25519.h
>  create mode 100644 include/zinc/poly1305.h
>  create mode 100644 include/zinc/simd.h
>  create mode 100644 lib/zinc/Makefile
>  create mode 100644 lib/zinc/blake2s/blake2s-x86_64.S
>  create mode 100644 lib/zinc/blake2s/blake2s.c
>  create mode 100644 lib/zinc/chacha20/chacha20-arm.S
>  create mode 100644 lib/zinc/chacha20/chacha20-arm64.S
>  create mode 100644 lib/zinc/chacha20/chacha20-mips.S
>  create mode 100644 lib/zinc/chacha20/chacha20-x86_64.S
>  create mode 100644 lib/zinc/chacha20/chacha20.c
>  create mode 100644 lib/zinc/chacha20poly1305.c
>  create mode 100644 lib/zinc/curve25519/curve25519-arm.S
>  create mode 100644 lib/zinc/curve25519/curve25519-arm.h
>  create mode 100644 lib/zinc/curve25519/curve25519-fiat32.h
>  create mode 100644 lib/zinc/curve25519/curve25519-hacl64.h
>  create mode 100644 lib/zinc/curve25519/curve25519-x86_64.h
>  create mode 100644 lib/zinc/curve25519/curve25519.c
>  create mode 100644 lib/zinc/main.c
>  create mode 100644 lib/zinc/poly1305/poly1305-arm.S
>  create mode 100644 lib/zinc/poly1305/poly1305-arm64.S
>  create mode 100644 lib/zinc/poly1305/poly1305-mips.S
>  create mode 100644 lib/zinc/poly1305/poly1305-mips64.S
>  create mode 100644 lib/zinc/poly1305/poly1305-x86_64.S
>  create mode 100644 lib/zinc/poly1305/poly1305.c
>  create mode 100644 lib/zinc/selftest/blake2s.h
>  create mode 100644 lib/zinc/selftest/chacha20poly1305.h
>  create mode 100644 lib/zinc/selftest/curve25519.h
>  create mode 100644 lib/zinc/selftest/poly1305.h
[...]

In general this is great work, and I'm very excited for WireGuard to be
upstreamed!  But for the new crypto code, I think a few things are on
the wrong track, for example treating it is a special library.  Even the
name is contradicting itself: Zinc is "not crypto/", yet as you stated
it's intended that the "Zinc" algorithms will be exposed through the
crypto API -- just like how most of the existing crypto code in lib/ is
also exposed through the crypto API.  So, I think that what you're doing
isn't actually *that* different from what already exists in some cases;
and pretending that it is very different is just going to cause
problems.  Rather, the actual truly new thing seems to be that the
dispatch to architecture specific implementations is done at the lib/
level instead of handled by the crypto API priority numbers.

So, I don't see why you don't just add lib/blake2s/, lib/chacha20/,
lib/poly1305/, etc., without pretending that they all have some special
new "Zinc" thing in common and are part of some holy crusade against the
crypto API.

They could even still go in subdirectory lib/crypto/ -- but just for
logical code organization purposes, as opposed to a special library with
a name that isn't self-explanatory and sounds like some third-party
library rather than first-class kernel code.

CONFIG_ZINC also needs to go.  Algorithms will need to be independently
configurable as soon as anything other than WireGuard needs to use any
of them, so you might as well do it right from the start with
CONFIG_BLAKE2, CONFIG_CHACHA20, CONFIG_POLY1305, etc.

I think the above changes would also naturally lead to a much saner
patch series where each algorithm is added by its own patch, rather than
one monster patch that adds many algorithms and 24000 lines of code.

Note that adding all the algorithms in one patch also makes the
description of them conflated, e.g. you wrote that "formally verified"
implementations were used whenever possible, but AFAICS that actually
only applies to the C implementations of Curve25519, and even those have
your copyright statement so presumably you had to change something from
the "formally verified" code :-).  Note also that Poly1305
implementations are somewhat error-prone, since there can be overflow
bugs that are extremely rarely hit; see e.g. how OpenSSL's Poly1305 NEON
implementation was initially buggy and had to be fixed:
https://mta.openssl.org/pipermail/openssl-commits/2016-April/006639.html.
Not to mention that C glue code is error-prone, especially with the tons
of #ifdefs.  So, I'd strongly prefer that you don't oversell the crypto
code you're adding, e.g. by implying that most of it is formally
verified, as it likely still has bugs, like any other code...

I also want to compare the performance of some of the assembly
implementations you're adding to the existing implementations in the
kernel they duplicate.  I'm especially interested in the NEON
implementation of ChaCha20.  But adding 11 implementations in one single
patch means there isn't really a chance to comment on them individually.

Also, earlier when I tested OpenSSL's ChaCha NEON implementation on ARM
Cortex-A7 it was actually quite a bit slower than the one in the Linux
kernel written by Ard Biesheuvel... I trust that when claiming the
performance of all implementations you're adding is "state-of-the-art
and unrivaled", you actually compared them to the ones already in the
Linux kernel which you're advocating replacing, right? :-)

Your patch description is also missing any mention of crypto accelerator
hardware.  Quite a bit of the complexity in the crypto API, such as
scatterlist support and asynchronous execution, exists because it
supports crypto accelerators.  AFAICS your new APIs cannot support
crypto accelerators, as your APIs are synchronous and operate on virtual
addresses.  I assume your justification is that "djb algorithms" like
ChaCha and Poly1305 don't need crypto accelerators as they are fast in
software.  But you never explicitly stated this and discussed the
tradeoffs.  Since this is basically the foundation for the design you've
chosen, it really needs to be addressed.

As for doing the architecture-specific dispatch in lib/ rather than
through the crypto API, there definitely are some arguments in favor of
it.  The main problem, though, is that your code is a mess due to all
the #ifdefs, and it will only get worse as people add more
architectures.  You may think you already added all the architectures
that matter, but tomorrow people will come and want to add PowerPC,
RISC-V, etc.  I really think you should consider splitting up
implementations by architecture; this would *not*, however, preclude the
implementations from still being accessed through a single top-level
"glue" function.  For example chacha20() could look like:

void chacha20(struct chacha20_ctx *state, u8 *dst, const u8 *src, u32 len,
	      bool have_simd)
{
	if (chacha20_arch(dst, src, len, state->key, state->counter, have_simd))
		goto out;

	chacha20_generic(dst, src, len, state->key, state->counter);

out:
	state->counter[0] += (len + 63) / 64;
}

So, each architecture would optionally define its own chacha20_arch()
that returns true if the data was processed, or false if not.  (The data
wouldn't be processed if, for example, 'have_simd' was false but only
SIMD implementations are available; or if the input was too short for an
SIMD implementation to be faster than the generic one.)  Note that this
would make the code much more consistent with the usual Linux kernel
coding style, which strongly prefers calling functions unconditionally
rather than having core logic littered with unmaintainable #ifdefs.

I'd also strongly prefer the patchset to include converting the crypto
API versions of ChaCha20 and Poly1305 over to use your new lib/ code, to
show that it's really possible.  You mentioned that it's planned, but if
it's not done right away there will be things that were missed and will
require changes when someone finally does it.  IMO it's not acceptable
to add your own completely separate ChaCha20 and Poly1305 just because
you don't like that the existing ones are part of the crypto API.  You
need to refactor things properly.  I think you'd need to expose the new
code under a cra_driver_name like "chacha20-software" and
"poly1305-software" to reflect that they use the fastest available
implementation of the algorithm on the CPU, e.g. "chacha20-generic",
"chacha20-neon", and "chacha20-simd" would all be replaced by a single
"chacha20-software".  Is that what you had in mind?

I'm also wondering about the origin and licensing of some of the
assembly language files.  Many have an OpenSSL copyright statement.
But, the OpenSSL license is often thought to be incompatible with GPL,
so using OpenSSL assembly code in the kernel has in the past required
getting special permission from Andy Polyakov (the person who's written
most of OpenSSL's assembly code so holds the copyright on it).  As one
example, see arch/arm/crypto/sha256-armv4.pl: the file explicitly states
that Andy has relicensed it under GPLv2.  For your new OpenSSL-derived
files, have you gone through and explicitly gotten GPLv2 permission from
Andy / the copyright holders?

Each assembly language file should also explicitly state where it came
from.  For example lib/zinc/curve25519/curve25519-arm.S has your
copyright statement and says it's "Based on algorithms from Daniel J.
Bernstein and Peter Schwabe.", but it's not clarified whether the *code*
was written by those other people, as opposed to those people designing
the *algorithms* and then you writing the code; and if you didn't write
it, where you retrieved the file from and when, what license it had
(even if it was "public domain" like some of djb's code, this should be
mentioned for informational purposes), and what changes you made if any.

Oh, and please use 80-character lines like the rest of the kernel, so
that people's eyes don't bleed when reading your code :-)

Thanks for all your hard work on WireGuard!

- Eric

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox