Re: [PATCH 2/3] xen netback: add fault injection facility

[Wei Liu]

On Fri, Apr 20, 2018 at 10:47:31AM +, Stanislav Kinsburskii wrote:
>  
>  #include 
>  #include 
> @@ -1649,6 +1650,7 @@ static int __init netback_init(void)
>   PTR_ERR(xen_netback_dbg_root));
>  #endif /* CONFIG_DEBUG_FS */
>  
> + (void) xen_netbk_fi_init();

If you care about the return value, please propagate it to
netback_init's caller. Otherwise you can just make the function return
void.

> +
> +int xenvif_fi_init(struct xenvif *vif)
> +{
> + struct dentry *parent;
> + struct xenvif_fi *vfi;
> + int fi, err = -ENOMEM;
> +
> + parent = vif_fi_dir;
> + if (!parent)
> + return -ENOMEM;
> +
> + vfi = kmalloc(sizeof(*vfi), GFP_KERNEL);
> + if (!vfi)
> + return -ENOMEM;
> +
> + vfi->dir = debugfs_create_dir(vif->dev->name, parent);
> + if (!vfi->dir)
> + goto err_dir;
> +
> + for (fi = 0; fi < XENVIF_FI_MAX; fi++) {
> + vfi->faults[fi] = xen_fi_dir_add(vfi->dir,
> + xenvif_fi_names[fi]);
> + if (!vfi->faults[fi])
> + goto err_fault;
> + }
> +
> + vif->fi_info = vfi;
> + return 0;
> +
> +err_fault:
> + for (; fi > 0; fi--)

fi >= 0

Wei.

Re: [PATCH 2/3] xen netback: add fault injection facility

[staskins]

On 04/20/18 15:00, Juergen Gross wrote:

On 20/04/18 14:52, stask...@amazon.com wrote:

On 04/20/18 13:25, Juergen Gross wrote:

On 20/04/18 12:47, Stanislav Kinsburskii wrote:

+    for (fi = 0; fi < XENVIF_FI_MAX; fi++) {
+    vfi->faults[fi] = xen_fi_dir_add(vfi->dir,
+    xenvif_fi_names[fi]);

How does this work? xenvif_fi_names[] is an empty array and this is the
only reference to it. Who is allocating the memory for that array?

Well, it works in the way one adds a var to enum (which is used as a key
later) and a corresponding string into the array (which is used as a
name for the fault directory in sysfs).

Then you should size the array via XENVIF_FI_MAX.


Makes sense.
Thanks!

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 2/3] xen netback: add fault injection facility

[Juergen Gross]

On 20/04/18 14:52, stask...@amazon.com wrote:
> On 04/20/18 13:25, Juergen Gross wrote:
>> On 20/04/18 12:47, Stanislav Kinsburskii wrote:
>>> +    for (fi = 0; fi < XENVIF_FI_MAX; fi++) {
>>> +    vfi->faults[fi] = xen_fi_dir_add(vfi->dir,
>>> +    xenvif_fi_names[fi]);
>> How does this work? xenvif_fi_names[] is an empty array and this is the
>> only reference to it. Who is allocating the memory for that array?
> 
> Well, it works in the way one adds a var to enum (which is used as a key
> later) and a corresponding string into the array (which is used as a
> name for the fault directory in sysfs).

Then you should size the array via XENVIF_FI_MAX.


Juergen

Re: [PATCH 2/3] xen netback: add fault injection facility

[staskins]

On 04/20/18 13:25, Juergen Gross wrote:

On 20/04/18 12:47, Stanislav Kinsburskii wrote:

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 8918466..5cc9acd 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -465,6 +465,14 @@ config XEN_NETDEV_BACKEND
  compile this driver as a module, chose M here: the module
  will be called xen-netback.
  
+config XEN_NETDEV_BACKEND_FAULT_INJECTION

+ bool "Xen net-device backend driver fault injection"
+ depends on XEN_NETDEV_BACKEND
+ depends on XEN_FAULT_INJECTION
+ default n
+ help
+   Allow to inject errors to Xen backend network driver
+
  config VMXNET3
tristate "VMware VMXNET3 ethernet driver"
depends on PCI && INET
diff --git a/drivers/net/xen-netback/Makefile b/drivers/net/xen-netback/Makefile
index d49798a..28abcdc 100644
--- a/drivers/net/xen-netback/Makefile
+++ b/drivers/net/xen-netback/Makefile
@@ -1,3 +1,4 @@
  obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o
  
  xen-netback-y := netback.o xenbus.o interface.o hash.o rx.o

+xen-netback-$(CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION) += netback_fi.o
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a46a1e9..30d676d 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -286,6 +286,9 @@ struct xenvif {
  
  #ifdef CONFIG_DEBUG_FS

struct dentry *xenvif_dbg_root;
+#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
+   void *fi_info;
+#endif
  #endif
  
  	struct xen_netif_ctrl_back_ring ctrl;

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index a27daa2..ecc416e 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -33,6 +33,7 @@
   */
  
  #include "common.h"

+#include "netback_fi.h"
  
  #include 

  #include 
@@ -1649,6 +1650,7 @@ static int __init netback_init(void)
PTR_ERR(xen_netback_dbg_root));
  #endif /* CONFIG_DEBUG_FS */
  
+	(void) xen_netbk_fi_init();

This is the only usage of xen_netbk_fi_init(). Why don't you make it
return void from the beginning?


Well, I could do so, of course.
My intention was to treat this as an error. But then it doesn't 
correlate to ignored debugfs directory creation error above.



return 0;
  
  failed_init:

@@ -1659,6 +1661,7 @@ module_init(netback_init);
  
  static void __exit netback_fini(void)

  {
+   xen_netbk_fi_fini();
  #ifdef CONFIG_DEBUG_FS
if (!IS_ERR_OR_NULL(xen_netback_dbg_root))
debugfs_remove_recursive(xen_netback_dbg_root);
diff --git a/drivers/net/xen-netback/netback_fi.c 
b/drivers/net/xen-netback/netback_fi.c
new file mode 100644
index 000..47541d0
--- /dev/null
+++ b/drivers/net/xen-netback/netback_fi.c
@@ -0,0 +1,119 @@
+/*
+ * Fault injection interface for Xen backend network driver
+ *
+ * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:

SPDX again.


Will fix.




+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "common.h"
+
+#include 
+
+#include 
+#include "netback_fi.h"
+
+static struct dentry *vif_fi_dir;
+
+static const char *xenvif_fi_names[] = {
+};
+
+struct xenvif_fi {
+   struct dentry *dir;
+   struct xen_fi *faults[XENVIF_FI_MAX];
+};
+
+int xen_netbk_fi_init(void)
+{
+   vif_fi_dir = xen_fi_dir_create("xen-netback");
+   if (!vif_fi_dir)
+   return -ENOMEM;
+   return 0;
+}
+
+void xen_netbk_fi_fini(void)
+{
+   debugfs_remove_recursive(vif_fi_dir);
+}
+
+void xenvif_fi_fini(struct xenvif *vif)
+{
+   struct 

Re: [PATCH 2/3] xen netback: add fault injection facility

[Juergen Gross]

On 20/04/18 12:47, Stanislav Kinsburskii wrote:
> This patch adds wrapper helpers around generic Xen fault inject facility.
> The major reason is to keep all the module fault injection directories
> in a dedicated subdirectory instead of Xen fault inject root.
> 
> IOW, when using these helpers, per-device and named by device name fault
> injection control directories will appear under the following directory:
> - /sys/kernel/debug/xen/fault_inject/xen-netback/
> instead of:
> - /sys/kernel/debug/xen/fault_inject/
> 
> Signed-off-by: Stanislav Kinsburskii 
> CC: Wei Liu 
> CC: Paul Durrant 
> CC: "David S. Miller" 
> CC: Matteo Croce 
> CC: Stefan Hajnoczi 
> CC: Daniel Borkmann 
> CC: Gerard Garcia 
> CC: David Ahern 
> CC: Juergen Gross 
> CC: Amir Levy 
> CC: Jakub Kicinski 
> CC: linux-ker...@vger.kernel.org
> CC: net...@vger.kernel.org
> CC: xen-de...@lists.xenproject.org
> CC: Stanislav Kinsburskii 
> CC: David Woodhouse 
> ---
>  drivers/net/Kconfig  |8 ++
>  drivers/net/xen-netback/Makefile |1 
>  drivers/net/xen-netback/common.h |3 +
>  drivers/net/xen-netback/netback.c|3 +
>  drivers/net/xen-netback/netback_fi.c |  119 
> ++
>  drivers/net/xen-netback/netback_fi.h |   35 ++
>  drivers/net/xen-netback/xenbus.c |6 ++
>  7 files changed, 175 insertions(+)
>  create mode 100644 drivers/net/xen-netback/netback_fi.c
>  create mode 100644 drivers/net/xen-netback/netback_fi.h
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 8918466..5cc9acd 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -465,6 +465,14 @@ config XEN_NETDEV_BACKEND
> compile this driver as a module, chose M here: the module
> will be called xen-netback.
>  
> +config XEN_NETDEV_BACKEND_FAULT_INJECTION
> +   bool "Xen net-device backend driver fault injection"
> +   depends on XEN_NETDEV_BACKEND
> +   depends on XEN_FAULT_INJECTION
> +   default n
> +   help
> + Allow to inject errors to Xen backend network driver
> +
>  config VMXNET3
>   tristate "VMware VMXNET3 ethernet driver"
>   depends on PCI && INET
> diff --git a/drivers/net/xen-netback/Makefile 
> b/drivers/net/xen-netback/Makefile
> index d49798a..28abcdc 100644
> --- a/drivers/net/xen-netback/Makefile
> +++ b/drivers/net/xen-netback/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o
>  
>  xen-netback-y := netback.o xenbus.o interface.o hash.o rx.o
> +xen-netback-$(CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION) += netback_fi.o
> diff --git a/drivers/net/xen-netback/common.h 
> b/drivers/net/xen-netback/common.h
> index a46a1e9..30d676d 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -286,6 +286,9 @@ struct xenvif {
>  
>  #ifdef CONFIG_DEBUG_FS
>   struct dentry *xenvif_dbg_root;
> +#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
> + void *fi_info;
> +#endif
>  #endif
>  
>   struct xen_netif_ctrl_back_ring ctrl;
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index a27daa2..ecc416e 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -33,6 +33,7 @@
>   */
>  
>  #include "common.h"
> +#include "netback_fi.h"
>  
>  #include 
>  #include 
> @@ -1649,6 +1650,7 @@ static int __init netback_init(void)
>   PTR_ERR(xen_netback_dbg_root));
>  #endif /* CONFIG_DEBUG_FS */
>  
> + (void) xen_netbk_fi_init();

This is the only usage of xen_netbk_fi_init(). Why don't you make it
return void from the beginning?

>   return 0;
>  
>  failed_init:
> @@ -1659,6 +1661,7 @@ module_init(netback_init);
>  
>  static void __exit netback_fini(void)
>  {
> + xen_netbk_fi_fini();
>  #ifdef CONFIG_DEBUG_FS
>   if (!IS_ERR_OR_NULL(xen_netback_dbg_root))
>   debugfs_remove_recursive(xen_netback_dbg_root);
> diff --git a/drivers/net/xen-netback/netback_fi.c 
> b/drivers/net/xen-netback/netback_fi.c
> new file mode 100644
> index 000..47541d0
> --- /dev/null
> +++ b/drivers/net/xen-netback/netback_fi.c
> @@ -0,0 +1,119 @@
> +/*
> + * Fault injection interface for Xen backend network driver
> + *
> + * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the 

[PATCH 2/3] xen netback: add fault injection facility

[Stanislav Kinsburskii]

This patch adds wrapper helpers around generic Xen fault inject facility.
The major reason is to keep all the module fault injection directories
in a dedicated subdirectory instead of Xen fault inject root.

IOW, when using these helpers, per-device and named by device name fault
injection control directories will appear under the following directory:
- /sys/kernel/debug/xen/fault_inject/xen-netback/
instead of:
- /sys/kernel/debug/xen/fault_inject/

Signed-off-by: Stanislav Kinsburskii 
CC: Wei Liu 
CC: Paul Durrant 
CC: "David S. Miller" 
CC: Matteo Croce 
CC: Stefan Hajnoczi 
CC: Daniel Borkmann 
CC: Gerard Garcia 
CC: David Ahern 
CC: Juergen Gross 
CC: Amir Levy 
CC: Jakub Kicinski 
CC: linux-ker...@vger.kernel.org
CC: net...@vger.kernel.org
CC: xen-de...@lists.xenproject.org
CC: Stanislav Kinsburskii 
CC: David Woodhouse 
---
 drivers/net/Kconfig  |8 ++
 drivers/net/xen-netback/Makefile |1 
 drivers/net/xen-netback/common.h |3 +
 drivers/net/xen-netback/netback.c|3 +
 drivers/net/xen-netback/netback_fi.c |  119 ++
 drivers/net/xen-netback/netback_fi.h |   35 ++
 drivers/net/xen-netback/xenbus.c |6 ++
 7 files changed, 175 insertions(+)
 create mode 100644 drivers/net/xen-netback/netback_fi.c
 create mode 100644 drivers/net/xen-netback/netback_fi.h

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 8918466..5cc9acd 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -465,6 +465,14 @@ config XEN_NETDEV_BACKEND
  compile this driver as a module, chose M here: the module
  will be called xen-netback.
 
+config XEN_NETDEV_BACKEND_FAULT_INJECTION
+ bool "Xen net-device backend driver fault injection"
+ depends on XEN_NETDEV_BACKEND
+ depends on XEN_FAULT_INJECTION
+ default n
+ help
+   Allow to inject errors to Xen backend network driver
+
 config VMXNET3
tristate "VMware VMXNET3 ethernet driver"
depends on PCI && INET
diff --git a/drivers/net/xen-netback/Makefile b/drivers/net/xen-netback/Makefile
index d49798a..28abcdc 100644
--- a/drivers/net/xen-netback/Makefile
+++ b/drivers/net/xen-netback/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_XEN_NETDEV_BACKEND) := xen-netback.o
 
 xen-netback-y := netback.o xenbus.o interface.o hash.o rx.o
+xen-netback-$(CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION) += netback_fi.o
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a46a1e9..30d676d 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -286,6 +286,9 @@ struct xenvif {
 
 #ifdef CONFIG_DEBUG_FS
struct dentry *xenvif_dbg_root;
+#ifdef CONFIG_XEN_NETDEV_BACKEND_FAULT_INJECTION
+   void *fi_info;
+#endif
 #endif
 
struct xen_netif_ctrl_back_ring ctrl;
diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index a27daa2..ecc416e 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -33,6 +33,7 @@
  */
 
 #include "common.h"
+#include "netback_fi.h"
 
 #include 
 #include 
@@ -1649,6 +1650,7 @@ static int __init netback_init(void)
PTR_ERR(xen_netback_dbg_root));
 #endif /* CONFIG_DEBUG_FS */
 
+   (void) xen_netbk_fi_init();
return 0;
 
 failed_init:
@@ -1659,6 +1661,7 @@ module_init(netback_init);
 
 static void __exit netback_fini(void)
 {
+   xen_netbk_fi_fini();
 #ifdef CONFIG_DEBUG_FS
if (!IS_ERR_OR_NULL(xen_netback_dbg_root))
debugfs_remove_recursive(xen_netback_dbg_root);
diff --git a/drivers/net/xen-netback/netback_fi.c 
b/drivers/net/xen-netback/netback_fi.c
new file mode 100644
index 000..47541d0
--- /dev/null
+++ b/drivers/net/xen-netback/netback_fi.c
@@ -0,0 +1,119 @@
+/*
+ * Fault injection interface for Xen backend network driver
+ *
+ * Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software