* [PATCH -net] zd1211rw: zd_usb: Use "%zu" to format size_t
From: Geert Uytterhoeven @ 2019-09-19 9:15 UTC (permalink / raw)
To: Gustavo A . R . Silva, Daniel Drake, Ulrich Kunitz, Kalle Valo,
David S . Miller
Cc: linux-wireless, netdev, linux-kernel, Geert Uytterhoeven
On 32-bit:
drivers/net/wireless/zydas/zd1211rw/zd_usb.c: In function ‘check_read_regs’:
drivers/net/wireless/zydas/zd1211rw/zd_def.h:18:25: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
dev_printk(level, dev, "%s() " fmt, __func__, ##args)
^~~~~~~
drivers/net/wireless/zydas/zd1211rw/zd_def.h:22:4: note: in expansion of macro ‘dev_printk_f’
dev_printk_f(KERN_DEBUG, dev, fmt, ## args)
^~~~~~~~~~~~
drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1635:3: note: in expansion of macro ‘dev_dbg_f’
dev_dbg_f(zd_usb_dev(usb),
^~~~~~~~~
drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1636:51: note: format string is defined here
"error: actual length %d less than expected %ld\n",
~~^
%d
Fixes: 84b0b66352470e64 ("zd1211rw: zd_usb: Use struct_size() helper")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
index 4e44ea8c652d65aa..7b5c2fe5bd4d9cde 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
@@ -1633,7 +1633,7 @@ static bool check_read_regs(struct zd_usb *usb, struct usb_req_read_regs *req,
*/
if (rr->length < struct_size(regs, regs, count)) {
dev_dbg_f(zd_usb_dev(usb),
- "error: actual length %d less than expected %ld\n",
+ "error: actual length %d less than expected %zu\n",
rr->length, struct_size(regs, regs, count));
return false;
}
--
2.17.1
^ permalink raw reply related
* Re: [bpf-next,v3] samples: bpf: add max_pckt_size option at xdp_adjust_tail
From: Daniel T. Lee @ 2019-09-19 9:16 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: Daniel Borkmann, Alexei Starovoitov, Networking, bpf
In-Reply-To: <CAEf4BzY_EAf9pH7YvL9XAXPUr9+g5Q7N_n45XBufdxkfDbf3aQ@mail.gmail.com>
On Thu, Sep 19, 2019 at 3:00 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 18, 2019 at 10:37 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Tue, Sep 17, 2019 at 1:04 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Sep 11, 2019 at 2:33 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > > >
> > > > Currently, at xdp_adjust_tail_kern.c, MAX_PCKT_SIZE is limited
> > > > to 600. To make this size flexible, a new map 'pcktsz' is added.
> > > >
> > > > By updating new packet size to this map from the userland,
> > > > xdp_adjust_tail_kern.o will use this value as a new max_pckt_size.
> > > >
> > > > If no '-P <MAX_PCKT_SIZE>' option is used, the size of maximum packet
> > > > will be 600 as a default.
> > > >
> > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > >
> > > > ---
> > > > Changes in v2:
> > > > - Change the helper to fetch map from 'bpf_map__next' to
> > > > 'bpf_object__find_map_fd_by_name'.
> > > >
> > > > samples/bpf/xdp_adjust_tail_kern.c | 23 +++++++++++++++++++----
> > > > samples/bpf/xdp_adjust_tail_user.c | 28 ++++++++++++++++++++++------
> > > > 2 files changed, 41 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/samples/bpf/xdp_adjust_tail_kern.c b/samples/bpf/xdp_adjust_tail_kern.c
> > > > index 411fdb21f8bc..d6d84ffe6a7a 100644
> > > > --- a/samples/bpf/xdp_adjust_tail_kern.c
> > > > +++ b/samples/bpf/xdp_adjust_tail_kern.c
> > > > @@ -25,6 +25,13 @@
> > > > #define ICMP_TOOBIG_SIZE 98
> > > > #define ICMP_TOOBIG_PAYLOAD_SIZE 92
> > > >
> > > > +struct bpf_map_def SEC("maps") pcktsz = {
> > > > + .type = BPF_MAP_TYPE_ARRAY,
> > > > + .key_size = sizeof(__u32),
> > > > + .value_size = sizeof(__u32),
> > > > + .max_entries = 1,
> > > > +};
> > > > +
> > >
> > > Hey Daniel,
> > >
> > > This looks like an ideal use case for global variables on BPF side. I
> > > think it's much cleaner and will make BPF side of things simpler.
> > > Would you mind giving global data a spin instead of adding this map?
> > >
> >
> > Sure thing!
> > But, I'm not sure there is global variables for BPF?
> > AFAIK, there aren't any support for global variables yet in BPF
> > program (_kern.c).
> >
> > # when defining global variable at _kern.c
> > libbpf: bpf: relocation: not yet supported relo for non-static
> > global '<var>' variable found in insns[39].code 0x18
>
> just what it says: use static global variable (also volatile to
> prevent compiler optimizations) :)
>
> static volatile __u32 pcktsz; /* this should work */
>
My apologies, but I'm not sure I'm following.
What you are saying is, should I define global variable to _kern,c
and access and modify this variable from _user.c?
For example,
<_kern.c>
static volatile __u32 pcktsz = 300;
<_user.c>
extern __u32 pcktsz;
// Later in code
pcktsz = 400;
Is this code means similar to what you've said?
AFAIK, 'static' keyword for global variable restricts scope to file itself,
so the 'accessing' and 'modifying' this variable from the <_user.c>
isn't available.
The reason why I've used bpf map for this 'pcktsz' option is,
I've wanted to run this kernel xdp program (xdp_adjust_tail_kern.o)
as it itself, not heavily controlled by user program (./xdp_adjust_tail).
When this 'pcktsz' option is implemented in bpf map, user can simply
modify 'map' to change this size. (such as bpftool prog map)
But when this variable comes to global data, it can't be changed
after the program gets loaded.
I really appreciate your time and effort for the review.
But I'm sorry that I seem to get it wrong.
Thanks,
Daniel
> >
> > By the way, thanks for the review.
> >
> > Thanks,
> > Daniel
> >
> >
> > > > struct bpf_map_def SEC("maps") icmpcnt = {
> > > > .type = BPF_MAP_TYPE_ARRAY,
> > > > .key_size = sizeof(__u32),
> > > > @@ -64,7 +71,8 @@ static __always_inline void ipv4_csum(void *data_start, int data_size,
> > > > *csum = csum_fold_helper(*csum);
> > > > }
> > > >
> > >
> > > [...]
^ permalink raw reply
* [PATCH v8 1/7] nfc: pn533: i2c: "pn532" as dt compatible string
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: Steve Winslow, Thomas Gleixner, Kate Stewart, Allison Randal,
Jilayne Lovejoy, Lars Poeschel, open list:NFC SUBSYSTEM,
open list
Cc: Johan Hovold, Simon Horman
It is favourable to have one unified compatible string for devices that
have multiple interfaces. So this adds simply "pn532" as the devicetree
binding compatible string and makes a note that the old ones are
deprecated.
Cc: Johan Hovold <johan@kernel.org>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5
Changes in v3:
- This patch is new in v3
drivers/nfc/pn533/i2c.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 1832cd921ea7..1abd40398a5a 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -245,6 +245,11 @@ static int pn533_i2c_remove(struct i2c_client *client)
}
static const struct of_device_id of_pn533_i2c_match[] = {
+ { .compatible = "nxp,pn532", },
+ /*
+ * NOTE: The use of the compatibles with the trailing "...-i2c" is
+ * deprecated and will be removed.
+ */
{ .compatible = "nxp,pn533-i2c", },
{ .compatible = "nxp,pn532-i2c", },
{},
--
2.23.0
^ permalink raw reply related
* [PATCH v8 2/7] nfc: pn532: Add uart phy docs and rename it
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: David S. Miller, Rob Herring, Mark Rutland,
GitAuthor: Lars Poeschel, open list:NFC SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Cc: Johan Hovold, Simon Horman
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
This adds documentation about the uart phy to the pn532 binding doc. As
the filename "pn533-i2c.txt" is not appropriate any more, rename it to
the more general "pn532.txt".
This also documents the deprecation of the compatible strings ending
with "...-i2c".
Cc: Johan Hovold <johan@kernel.org>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v8:
- Update existing binding doc instead of adding a new one:
- Add uart phy example
- Add general "pn532" compatible string
- Deprecate "...-i2c" compatible strings
- Rename file to a more general filename
- Intentionally drop Rob's Reviewed-By as I guess this rather big change
requires a new review
Changes in v7:
- Accidentally lost Rob's Reviewed-By
Changes in v6:
- Rebased the patch series on v5.3-rc5
- Picked up Rob's Reviewed-By
Changes in v4:
- Add documentation about reg property in case of i2c
Changes in v3:
- seperate binding doc instead of entry in trivial-devices.txt
.../devicetree/bindings/net/nfc/pn532.txt | 46 +++++++++++++++++++
.../devicetree/bindings/net/nfc/pn533-i2c.txt | 29 ------------
2 files changed, 46 insertions(+), 29 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/nfc/pn532.txt
delete mode 100644 Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
diff --git a/Documentation/devicetree/bindings/net/nfc/pn532.txt b/Documentation/devicetree/bindings/net/nfc/pn532.txt
new file mode 100644
index 000000000000..f0591f160bee
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nfc/pn532.txt
@@ -0,0 +1,46 @@
+* NXP Semiconductors PN532 NFC Controller
+
+Required properties:
+- compatible: Should be
+ - "nxp,pn532" Place a node with this inside the devicetree node of the bus
+ where the NFC chip is connected to.
+ Currently the kernel has phy bindings for uart and i2c.
+ - "nxp,pn532-i2c" (DEPRECATED) only works for the i2c binding.
+ - "nxp,pn533-i2c" (DEPRECATED) only works for the i2c binding.
+
+Required properties if connected on i2c:
+- clock-frequency: I²C work frequency.
+- reg: for the I²C bus address. This is fixed at 0x24 for the PN532.
+- interrupts: GPIO interrupt to which the chip is connected
+
+Optional SoC Specific Properties:
+- pinctrl-names: Contains only one value - "default".
+- pintctrl-0: Specifies the pin control groups used for this controller.
+
+Example (for ARM-based BeagleBone with PN532 on I2C2):
+
+&i2c2 {
+
+
+ pn532: pn532@24 {
+
+ compatible = "nxp,pn532";
+
+ reg = <0x24>;
+ clock-frequency = <400000>;
+
+ interrupt-parent = <&gpio1>;
+ interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
+
+ };
+};
+
+Example (for PN532 connected via uart):
+
+uart4: serial@49042000 {
+ compatible = "ti,omap3-uart";
+
+ pn532: nfc {
+ compatible = "nxp,pn532";
+ };
+};
diff --git a/Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt b/Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
deleted file mode 100644
index 2efe3886b95b..000000000000
--- a/Documentation/devicetree/bindings/net/nfc/pn533-i2c.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-* NXP Semiconductors PN532 NFC Controller
-
-Required properties:
-- compatible: Should be "nxp,pn532-i2c" or "nxp,pn533-i2c".
-- clock-frequency: I²C work frequency.
-- reg: address on the bus
-- interrupts: GPIO interrupt to which the chip is connected
-
-Optional SoC Specific Properties:
-- pinctrl-names: Contains only one value - "default".
-- pintctrl-0: Specifies the pin control groups used for this controller.
-
-Example (for ARM-based BeagleBone with PN532 on I2C2):
-
-&i2c2 {
-
-
- pn532: pn532@24 {
-
- compatible = "nxp,pn532-i2c";
-
- reg = <0x24>;
- clock-frequency = <400000>;
-
- interrupt-parent = <&gpio1>;
- interrupts = <17 IRQ_TYPE_EDGE_FALLING>;
-
- };
-};
--
2.23.0
^ permalink raw reply related
* [PATCH v8 3/7] nfc: pn533: Add dev_up/dev_down hooks to phy_ops
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: Thomas Gleixner, Kees Cook, Greg Kroah-Hartman, Allison Randal,
Jilayne Lovejoy, Lars Poeschel, Gustavo A. R. Silva,
Steve Winslow, open list:NFC SUBSYSTEM, open list
Cc: Johan Hovold
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
This adds hooks for dev_up and dev_down to the phy_ops. They are
optional.
The idea is to inform the phy driver when the nfc chip is really going
to be used. When it is not used, the phy driver can suspend it's
interface to the nfc chip to save some power. The nfc chip is considered
not in use before dev_up and after dev_down.
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5
Changes in v5:
- (dev->phy_ops->dev_up) instead (dev->phy_ops)
Changes in v4:
- This patch is new in v4
drivers/nfc/pn533/pn533.c | 12 +++++++++++-
drivers/nfc/pn533/pn533.h | 9 +++++++++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index a172a32aa9d9..64836c727aee 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2458,6 +2458,9 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
{
struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+ if (dev->phy_ops->dev_up)
+ dev->phy_ops->dev_up(dev);
+
if (dev->device_type == PN533_DEVICE_PN532) {
int rc = pn532_sam_configuration(nfc_dev);
@@ -2470,7 +2473,14 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
static int pn533_dev_down(struct nfc_dev *nfc_dev)
{
- return pn533_rf_field(nfc_dev, 0);
+ struct pn533 *dev = nfc_get_drvdata(nfc_dev);
+ int ret;
+
+ ret = pn533_rf_field(nfc_dev, 0);
+ if (dev->phy_ops->dev_down && !ret)
+ dev->phy_ops->dev_down(dev);
+
+ return ret;
}
static struct nfc_ops pn533_nfc_ops = {
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 8bf9d6ece0f5..570ee0a3e832 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -207,6 +207,15 @@ struct pn533_phy_ops {
struct sk_buff *out);
int (*send_ack)(struct pn533 *dev, gfp_t flags);
void (*abort_cmd)(struct pn533 *priv, gfp_t flags);
+ /*
+ * dev_up and dev_down are optional.
+ * They are used to inform the phy layer that the nfc chip
+ * is going to be really used very soon. The phy layer can then
+ * bring up it's interface to the chip and have it suspended for power
+ * saving reasons otherwise.
+ */
+ void (*dev_up)(struct pn533 *priv);
+ void (*dev_down)(struct pn533 *priv);
};
--
2.23.0
^ permalink raw reply related
* [PATCH v8 4/7] nfc: pn533: Split pn533 init & nfc_register
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: Thomas Gleixner, Jilayne Lovejoy, Kate Stewart, Steve Winslow,
Allison Randal, Lars Poeschel, Greg Kroah-Hartman,
Gustavo A. R. Silva, open list:NFC SUBSYSTEM, open list
Cc: Johan Hovold, Claudiu Beznea
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
There is a problem in the initialisation and setup of the pn533: It
registers with nfc too early. It could happen, that it finished
registering with nfc and someone starts using it. But setup of the pn533
is not yet finished. Bad or at least unintended things could happen.
So I split out nfc registering (and unregistering) to seperate functions
that have to be called late in probe then.
Cc: Johan Hovold <johan@kernel.org>
Cc: Claudiu Beznea <Claudiu.Beznea@microchip.com>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v7:
- Remove an unneeded rc variable initialization
- Corrected goto error to err_clean in pn533_usb_probe
Changes in v6:
- Rebased the patch series on v5.3-rc5
Changes in v5:
- This patch is new in v5
drivers/nfc/pn533/i2c.c | 17 +++++-----
drivers/nfc/pn533/pn533.c | 66 ++++++++++++++++++++-------------------
drivers/nfc/pn533/pn533.h | 11 ++++---
drivers/nfc/pn533/usb.c | 14 ++++++---
4 files changed, 60 insertions(+), 48 deletions(-)
diff --git a/drivers/nfc/pn533/i2c.c b/drivers/nfc/pn533/i2c.c
index 1abd40398a5a..e9e5a1ec8857 100644
--- a/drivers/nfc/pn533/i2c.c
+++ b/drivers/nfc/pn533/i2c.c
@@ -193,12 +193,10 @@ static int pn533_i2c_probe(struct i2c_client *client,
phy->i2c_dev = client;
i2c_set_clientdata(client, phy);
- priv = pn533_register_device(PN533_DEVICE_PN532,
- PN533_NO_TYPE_B_PROTOCOLS,
+ priv = pn53x_common_init(PN533_DEVICE_PN532,
PN533_PROTO_REQ_ACK_RESP,
phy, &i2c_phy_ops, NULL,
- &phy->i2c_dev->dev,
- &client->dev);
+ &phy->i2c_dev->dev);
if (IS_ERR(priv)) {
r = PTR_ERR(priv);
@@ -220,13 +218,17 @@ static int pn533_i2c_probe(struct i2c_client *client,
if (r)
goto fn_setup_err;
- return 0;
+ r = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &client->dev);
+ if (r)
+ goto fn_setup_err;
+
+ return r;
fn_setup_err:
free_irq(client->irq, phy);
irq_rqst_err:
- pn533_unregister_device(phy->priv);
+ pn53x_common_clean(phy->priv);
return r;
}
@@ -239,7 +241,8 @@ static int pn533_i2c_remove(struct i2c_client *client)
free_irq(client->irq, phy);
- pn533_unregister_device(phy->priv);
+ pn53x_unregister_nfc(phy->priv);
+ pn53x_common_clean(phy->priv);
return 0;
}
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index 64836c727aee..e5d5e4c83a04 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -2590,14 +2590,12 @@ int pn533_finalize_setup(struct pn533 *dev)
}
EXPORT_SYMBOL_GPL(pn533_finalize_setup);
-struct pn533 *pn533_register_device(u32 device_type,
- u32 protocols,
+struct pn533 *pn53x_common_init(u32 device_type,
enum pn533_protocol_type protocol_type,
void *phy,
struct pn533_phy_ops *phy_ops,
struct pn533_frame_ops *fops,
- struct device *dev,
- struct device *parent)
+ struct device *dev)
{
struct pn533 *priv;
int rc = -ENOMEM;
@@ -2638,43 +2636,18 @@ struct pn533 *pn533_register_device(u32 device_type,
skb_queue_head_init(&priv->fragment_skb);
INIT_LIST_HEAD(&priv->cmd_queue);
-
- priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
- priv->ops->tx_header_len +
- PN533_CMD_DATAEXCH_HEAD_LEN,
- priv->ops->tx_tail_len);
- if (!priv->nfc_dev) {
- rc = -ENOMEM;
- goto destroy_wq;
- }
-
- nfc_set_parent_dev(priv->nfc_dev, parent);
- nfc_set_drvdata(priv->nfc_dev, priv);
-
- rc = nfc_register_device(priv->nfc_dev);
- if (rc)
- goto free_nfc_dev;
-
return priv;
-free_nfc_dev:
- nfc_free_device(priv->nfc_dev);
-
-destroy_wq:
- destroy_workqueue(priv->wq);
error:
kfree(priv);
return ERR_PTR(rc);
}
-EXPORT_SYMBOL_GPL(pn533_register_device);
+EXPORT_SYMBOL_GPL(pn53x_common_init);
-void pn533_unregister_device(struct pn533 *priv)
+void pn53x_common_clean(struct pn533 *priv)
{
struct pn533_cmd *cmd, *n;
- nfc_unregister_device(priv->nfc_dev);
- nfc_free_device(priv->nfc_dev);
-
flush_delayed_work(&priv->poll_work);
destroy_workqueue(priv->wq);
@@ -2689,8 +2662,37 @@ void pn533_unregister_device(struct pn533 *priv)
kfree(priv);
}
-EXPORT_SYMBOL_GPL(pn533_unregister_device);
+EXPORT_SYMBOL_GPL(pn53x_common_clean);
+
+int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
+ struct device *parent)
+{
+ int rc;
+
+ priv->nfc_dev = nfc_allocate_device(&pn533_nfc_ops, protocols,
+ priv->ops->tx_header_len +
+ PN533_CMD_DATAEXCH_HEAD_LEN,
+ priv->ops->tx_tail_len);
+ if (!priv->nfc_dev)
+ return -ENOMEM;
+
+ nfc_set_parent_dev(priv->nfc_dev, parent);
+ nfc_set_drvdata(priv->nfc_dev, priv);
+
+ rc = nfc_register_device(priv->nfc_dev);
+ if (rc)
+ nfc_free_device(priv->nfc_dev);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(pn53x_register_nfc);
+void pn53x_unregister_nfc(struct pn533 *priv)
+{
+ nfc_unregister_device(priv->nfc_dev);
+ nfc_free_device(priv->nfc_dev);
+}
+EXPORT_SYMBOL_GPL(pn53x_unregister_nfc);
MODULE_AUTHOR("Lauro Ramos Venancio <lauro.venancio@openbossa.org>");
MODULE_AUTHOR("Aloisio Almeida Jr <aloisio.almeida@openbossa.org>");
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 570ee0a3e832..510ddebbd896 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -219,18 +219,19 @@ struct pn533_phy_ops {
};
-struct pn533 *pn533_register_device(u32 device_type,
- u32 protocols,
+struct pn533 *pn53x_common_init(u32 device_type,
enum pn533_protocol_type protocol_type,
void *phy,
struct pn533_phy_ops *phy_ops,
struct pn533_frame_ops *fops,
- struct device *dev,
- struct device *parent);
+ struct device *dev);
int pn533_finalize_setup(struct pn533 *dev);
-void pn533_unregister_device(struct pn533 *priv);
+void pn53x_common_clean(struct pn533 *priv);
void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status);
+int pn53x_register_nfc(struct pn533 *priv, u32 protocols,
+ struct device *parent);
+void pn53x_unregister_nfc(struct pn533 *priv);
bool pn533_rx_frame_is_cmd_response(struct pn533 *dev, void *frame);
bool pn533_rx_frame_is_ack(void *_frame);
diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
index c5289eaf17ee..8511fd089970 100644
--- a/drivers/nfc/pn533/usb.c
+++ b/drivers/nfc/pn533/usb.c
@@ -534,9 +534,9 @@ static int pn533_usb_probe(struct usb_interface *interface,
goto error;
}
- priv = pn533_register_device(id->driver_info, protocols, protocol_type,
+ priv = pn53x_common_init(id->driver_info, protocol_type,
phy, &usb_phy_ops, fops,
- &phy->udev->dev, &interface->dev);
+ &phy->udev->dev);
if (IS_ERR(priv)) {
rc = PTR_ERR(priv);
@@ -547,12 +547,17 @@ static int pn533_usb_probe(struct usb_interface *interface,
rc = pn533_finalize_setup(priv);
if (rc)
- goto error;
+ goto err_clean;
usb_set_intfdata(interface, phy);
+ rc = pn53x_register_nfc(priv, protocols, &interface->dev);
+ if (rc)
+ goto err_clean;
return 0;
+err_clean:
+ pn53x_common_clean(priv);
error:
usb_free_urb(phy->in_urb);
usb_free_urb(phy->out_urb);
@@ -570,7 +575,8 @@ static void pn533_usb_disconnect(struct usb_interface *interface)
if (!phy)
return;
- pn533_unregister_device(phy->priv);
+ pn53x_unregister_nfc(phy->priv);
+ pn53x_common_clean(phy->priv);
usb_set_intfdata(interface, NULL);
--
2.23.0
^ permalink raw reply related
* [PATCH v8 5/7] nfc: pn533: add UART phy driver
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: Thomas Gleixner, Greg Kroah-Hartman, Lars Poeschel,
Jilayne Lovejoy, Kate Stewart, Allison Randal, Steve Winslow,
open list, open list:NFC SUBSYSTEM
Cc: Johan Hovold, Claudiu Beznea, David Miller
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
This adds the UART phy interface for the pn533 driver.
The pn533 driver can be used through UART interface this way.
It is implemented as a serdev device.
Cc: Johan Hovold <johan@kernel.org>
Cc: Claudiu Beznea <Claudiu.Beznea@microchip.com>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v8:
- Reverse christmas tree order for local variables in
pn532_uart_send_ack and pn532_uart_rx_is_frame
Changes in v7:
- Add comment at send_wakeup variable to document a possible and
harmless concurrency issue
Changes in v6:
- Rebased the patch series on v5.3-rc5
Changes in v5:
- Use the splitted pn53x_common_init and pn53x_register_nfc
and pn53x_common_clean and pn53x_unregister_nfc alike
Changes in v4:
- SPDX-License-Identifier: GPL-2.0+
- Source code comments above refering items
- Error check for serdev_device_write's
- Change if (xxx == NULL) to if (!xxx)
- Remove device name from a dev_err
- move pn533_register in _probe a bit towards the end of _probe
- make use of newly added dev_up / dev_down phy_ops
- control send_wakeup variable from dev_up / dev_down
Changes in v3:
- depend on SERIAL_DEV_BUS in Kconfig
Changes in v2:
- switched from tty line discipline to serdev, resulting in many
simplifications
- SPDX License Identifier
drivers/nfc/pn533/Kconfig | 11 ++
drivers/nfc/pn533/Makefile | 2 +
drivers/nfc/pn533/pn533.h | 8 +
drivers/nfc/pn533/uart.c | 324 +++++++++++++++++++++++++++++++++++++
4 files changed, 345 insertions(+)
create mode 100644 drivers/nfc/pn533/uart.c
diff --git a/drivers/nfc/pn533/Kconfig b/drivers/nfc/pn533/Kconfig
index f6d6b345ba0d..7fe1bbe26568 100644
--- a/drivers/nfc/pn533/Kconfig
+++ b/drivers/nfc/pn533/Kconfig
@@ -26,3 +26,14 @@ config NFC_PN533_I2C
If you choose to build a module, it'll be called pn533_i2c.
Say N if unsure.
+
+config NFC_PN532_UART
+ tristate "NFC PN532 device support (UART)"
+ depends on SERIAL_DEV_BUS
+ select NFC_PN533
+ ---help---
+ This module adds support for the NXP pn532 UART interface.
+ Select this if your platform is using the UART bus.
+
+ If you choose to build a module, it'll be called pn532_uart.
+ Say N if unsure.
diff --git a/drivers/nfc/pn533/Makefile b/drivers/nfc/pn533/Makefile
index 43c25b4f9466..b9648337576f 100644
--- a/drivers/nfc/pn533/Makefile
+++ b/drivers/nfc/pn533/Makefile
@@ -4,7 +4,9 @@
#
pn533_usb-objs = usb.o
pn533_i2c-objs = i2c.o
+pn532_uart-objs = uart.o
obj-$(CONFIG_NFC_PN533) += pn533.o
obj-$(CONFIG_NFC_PN533_USB) += pn533_usb.o
obj-$(CONFIG_NFC_PN533_I2C) += pn533_i2c.o
+obj-$(CONFIG_NFC_PN532_UART) += pn532_uart.o
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 510ddebbd896..6541088fad73 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -43,6 +43,11 @@
/* Preamble (1), SoPC (2), ACK Code (2), Postamble (1) */
#define PN533_STD_FRAME_ACK_SIZE 6
+/*
+ * Preamble (1), SoPC (2), Packet Length (1), Packet Length Checksum (1),
+ * Specific Application Level Error Code (1) , Postamble (1)
+ */
+#define PN533_STD_ERROR_FRAME_SIZE 8
#define PN533_STD_FRAME_CHECKSUM(f) (f->data[f->datalen])
#define PN533_STD_FRAME_POSTAMBLE(f) (f->data[f->datalen + 1])
/* Half start code (3), LEN (4) should be 0xffff for extended frame */
@@ -84,6 +89,9 @@
#define PN533_CMD_MI_MASK 0x40
#define PN533_CMD_RET_SUCCESS 0x00
+#define PN533_FRAME_DATALEN_ACK 0x00
+#define PN533_FRAME_DATALEN_ERROR 0x01
+#define PN533_FRAME_DATALEN_EXTENDED 0xFF
enum pn533_protocol_type {
PN533_PROTO_REQ_ACK_RESP = 0,
diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
new file mode 100644
index 000000000000..7f639051cdd0
--- /dev/null
+++ b/drivers/nfc/pn533/uart.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Driver for NXP PN532 NFC Chip - UART transport layer
+ *
+ * Copyright (C) 2018 Lemonage Software GmbH
+ * Author: Lars Pöschel <poeschel@lemonage.de>
+ * All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/nfc.h>
+#include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/serdev.h>
+#include "pn533.h"
+
+#define PN532_UART_SKB_BUFF_LEN (PN533_CMD_DATAEXCH_DATA_MAXLEN * 2)
+
+enum send_wakeup {
+ PN532_SEND_NO_WAKEUP = 0,
+ PN532_SEND_WAKEUP,
+ PN532_SEND_LAST_WAKEUP,
+};
+
+
+struct pn532_uart_phy {
+ struct serdev_device *serdev;
+ struct sk_buff *recv_skb;
+ struct pn533 *priv;
+ /*
+ * send_wakeup variable is used to control if we need to send a wakeup
+ * request to the pn532 chip prior to our actual command. There is a
+ * little propability of a race condition. We decided to not mutex the
+ * variable as the worst that could happen is, that we send a wakeup
+ * to the chip that is already awake. This does not hurt. It is a
+ * no-op to the chip.
+ */
+ enum send_wakeup send_wakeup;
+ struct timer_list cmd_timeout;
+ struct sk_buff *cur_out_buf;
+};
+
+static int pn532_uart_send_frame(struct pn533 *dev,
+ struct sk_buff *out)
+{
+ /* wakeup sequence and dummy bytes for waiting time */
+ static const u8 wakeup[] = {
+ 0x55, 0x55, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+ struct pn532_uart_phy *pn532 = dev->phy;
+ int err;
+
+ print_hex_dump_debug("PN532_uart TX: ", DUMP_PREFIX_NONE, 16, 1,
+ out->data, out->len, false);
+
+ pn532->cur_out_buf = out;
+ if (pn532->send_wakeup) {
+ err= serdev_device_write(pn532->serdev,
+ wakeup, sizeof(wakeup),
+ MAX_SCHEDULE_TIMEOUT);
+ if (err < 0)
+ return err;
+ }
+
+ if (pn532->send_wakeup == PN532_SEND_LAST_WAKEUP) {
+ pn532->send_wakeup = PN532_SEND_NO_WAKEUP;
+ }
+
+ err = serdev_device_write(pn532->serdev, out->data, out->len,
+ MAX_SCHEDULE_TIMEOUT);
+ if (err < 0)
+ return err;
+
+ mod_timer(&pn532->cmd_timeout, HZ / 40 + jiffies);
+ return 0;
+}
+
+static int pn532_uart_send_ack(struct pn533 *dev, gfp_t flags)
+{
+ /* spec 7.1.1.3: Preamble, SoPC (2), ACK Code (2), Postamble */
+ static const u8 ack[PN533_STD_FRAME_ACK_SIZE] = {
+ 0x00, 0x00, 0xff, 0x00, 0xff, 0x00};
+ struct pn532_uart_phy *pn532 = dev->phy;
+ int err;
+
+ err = serdev_device_write(pn532->serdev, ack, sizeof(ack),
+ MAX_SCHEDULE_TIMEOUT);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
+static void pn532_uart_abort_cmd(struct pn533 *dev, gfp_t flags)
+{
+ /* An ack will cancel the last issued command */
+ pn532_uart_send_ack(dev, flags);
+ /* schedule cmd_complete_work to finish current command execution */
+ pn533_recv_frame(dev, NULL, -ENOENT);
+}
+
+static void pn532_dev_up(struct pn533 *dev)
+{
+ struct pn532_uart_phy *pn532 = dev->phy;
+
+ serdev_device_open(pn532->serdev);
+ pn532->send_wakeup = PN532_SEND_LAST_WAKEUP;
+}
+
+static void pn532_dev_down(struct pn533 *dev)
+{
+ struct pn532_uart_phy *pn532 = dev->phy;
+
+ serdev_device_close(pn532->serdev);
+ pn532->send_wakeup = PN532_SEND_WAKEUP;
+}
+
+static struct pn533_phy_ops uart_phy_ops = {
+ .send_frame = pn532_uart_send_frame,
+ .send_ack = pn532_uart_send_ack,
+ .abort_cmd = pn532_uart_abort_cmd,
+ .dev_up = pn532_dev_up,
+ .dev_down = pn532_dev_down,
+};
+
+static void pn532_cmd_timeout(struct timer_list *t)
+{
+ struct pn532_uart_phy *dev = from_timer(dev, t, cmd_timeout);
+
+ pn532_uart_send_frame(dev->priv, dev->cur_out_buf);
+}
+
+/*
+ * scans the buffer if it contains a pn532 frame. It is not checked if the
+ * frame is really valid. This is later done with pn533_rx_frame_is_valid.
+ * This is useful for malformed or errornous transmitted frames. Adjusts the
+ * bufferposition where the frame starts, since pn533_recv_frame expects a
+ * well formed frame.
+ */
+static int pn532_uart_rx_is_frame(struct sk_buff *skb)
+{
+ struct pn533_std_frame *std;
+ struct pn533_ext_frame *ext;
+ u16 frame_len;
+ int i;
+
+ for (i = 0; i + PN533_STD_FRAME_ACK_SIZE <= skb->len; i++) {
+ std = (struct pn533_std_frame *)&skb->data[i];
+ /* search start code */
+ if (std->start_frame != cpu_to_be16(PN533_STD_FRAME_SOF))
+ continue;
+
+ /* frame type */
+ switch (std->datalen) {
+ case PN533_FRAME_DATALEN_ACK:
+ if (std->datalen_checksum == 0xff) {
+ skb_pull(skb, i);
+ return 1;
+ }
+
+ break;
+ case PN533_FRAME_DATALEN_ERROR:
+ if ((std->datalen_checksum == 0xff) &&
+ (skb->len >=
+ PN533_STD_ERROR_FRAME_SIZE)) {
+ skb_pull(skb, i);
+ return 1;
+ }
+
+ break;
+ case PN533_FRAME_DATALEN_EXTENDED:
+ ext = (struct pn533_ext_frame *)&skb->data[i];
+ frame_len = ext->datalen;
+ if (skb->len >= frame_len +
+ sizeof(struct pn533_ext_frame) +
+ 2 /* CKS + Postamble */) {
+ skb_pull(skb, i);
+ return 1;
+ }
+
+ break;
+ default: /* normal information frame */
+ frame_len = std->datalen;
+ if (skb->len >= frame_len +
+ sizeof(struct pn533_std_frame) +
+ 2 /* CKS + Postamble */) {
+ skb_pull(skb, i);
+ return 1;
+ }
+
+ break;
+ }
+ }
+
+ return 0;
+}
+
+static int pn532_receive_buf(struct serdev_device *serdev,
+ const unsigned char *data, size_t count)
+{
+ struct pn532_uart_phy *dev = serdev_device_get_drvdata(serdev);
+ size_t i;
+
+ del_timer(&dev->cmd_timeout);
+ for (i = 0; i < count; i++) {
+ skb_put_u8(dev->recv_skb, *data++);
+ if (!pn532_uart_rx_is_frame(dev->recv_skb))
+ continue;
+
+ pn533_recv_frame(dev->priv, dev->recv_skb, 0);
+ dev->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
+ if (!dev->recv_skb)
+ return 0;
+ }
+
+ return i;
+}
+
+static struct serdev_device_ops pn532_serdev_ops = {
+ .receive_buf = pn532_receive_buf,
+ .write_wakeup = serdev_device_write_wakeup,
+};
+
+static const struct of_device_id pn532_uart_of_match[] = {
+ { .compatible = "nxp,pn532", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, pn532_uart_of_match);
+
+static int pn532_uart_probe(struct serdev_device *serdev)
+{
+ struct pn532_uart_phy *pn532;
+ struct pn533 *priv;
+ int err;
+
+ err = -ENOMEM;
+ pn532 = kzalloc(sizeof(*pn532), GFP_KERNEL);
+ if (!pn532)
+ goto err_exit;
+
+ pn532->recv_skb = alloc_skb(PN532_UART_SKB_BUFF_LEN, GFP_KERNEL);
+ if (!pn532->recv_skb)
+ goto err_free;
+
+ pn532->serdev = serdev;
+ serdev_device_set_drvdata(serdev, pn532);
+ serdev_device_set_client_ops(serdev, &pn532_serdev_ops);
+ err = serdev_device_open(serdev);
+ if (err) {
+ dev_err(&serdev->dev, "Unable to open device\n");
+ goto err_skb;
+ }
+
+ err = serdev_device_set_baudrate(serdev, 115200);
+ if (err != 115200) {
+ err = -EINVAL;
+ goto err_serdev;
+ }
+
+ serdev_device_set_flow_control(serdev, false);
+ pn532->send_wakeup = PN532_SEND_WAKEUP;
+ timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
+ priv = pn53x_common_init(PN533_DEVICE_PN532,
+ PN533_PROTO_REQ_ACK_RESP,
+ pn532, &uart_phy_ops, NULL,
+ &pn532->serdev->dev);
+ if (IS_ERR(priv)) {
+ err = PTR_ERR(priv);
+ goto err_serdev;
+ }
+
+ pn532->priv = priv;
+ err = pn533_finalize_setup(pn532->priv);
+ if (err)
+ goto err_clean;
+
+ serdev_device_close(serdev);
+ err = pn53x_register_nfc(priv, PN533_NO_TYPE_B_PROTOCOLS, &serdev->dev);
+ if (err) {
+ pn53x_common_clean(pn532->priv);
+ goto err_skb;
+ }
+
+ return err;
+
+err_clean:
+ pn53x_common_clean(pn532->priv);
+err_serdev:
+ serdev_device_close(serdev);
+err_skb:
+ kfree_skb(pn532->recv_skb);
+err_free:
+ kfree(pn532);
+err_exit:
+ return err;
+}
+
+static void pn532_uart_remove(struct serdev_device *serdev)
+{
+ struct pn532_uart_phy *pn532 = serdev_device_get_drvdata(serdev);
+
+ pn53x_unregister_nfc(pn532->priv);
+ serdev_device_close(serdev);
+ pn53x_common_clean(pn532->priv);
+ kfree_skb(pn532->recv_skb);
+ kfree(pn532);
+}
+
+static struct serdev_device_driver pn532_uart_driver = {
+ .probe = pn532_uart_probe,
+ .remove = pn532_uart_remove,
+ .driver = {
+ .name = "pn532_uart",
+ .of_match_table = of_match_ptr(pn532_uart_of_match),
+ },
+};
+
+module_serdev_device_driver(pn532_uart_driver);
+
+MODULE_AUTHOR("Lars Pöschel <poeschel@lemonage.de>");
+MODULE_DESCRIPTION("PN532 UART driver");
+MODULE_LICENSE("GPL");
--
2.23.0
^ permalink raw reply related
* [PATCH v8 6/7] nfc: pn533: Add autopoll capability
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: Jilayne Lovejoy, Gustavo A. R. Silva, Kate Stewart, Steve Winslow,
Kees Cook, Lars Poeschel, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, open list:NFC SUBSYSTEM, open list
Cc: Johan Hovold, David Miller
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
pn532 devices support an autopoll command, that lets the chip
automatically poll for selected nfc technologies instead of manually
looping through every single nfc technology the user is interested in.
This is faster and less cpu and bus intensive than manually polling.
This adds this autopoll capability to the pn533 driver.
Cc: Johan Hovold <johan@kernel.org>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v8:
- Reverse christmas tree order for local variables in
pn533_autopoll_complete and pn533_start_poll
Changes in v7:
- Remove __packed attribute at struct pn532_autopoll_resp
- Add missing '\n' at the end of dev_dbg and nfc_err strings
Changes in v6:
- Rebased the patch series on v5.3-rc5
drivers/nfc/pn533/pn533.c | 193 +++++++++++++++++++++++++++++++++++++-
drivers/nfc/pn533/pn533.h | 10 +-
2 files changed, 197 insertions(+), 6 deletions(-)
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index e5d5e4c83a04..044b52d036e3 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -185,6 +185,32 @@ struct pn533_cmd_jump_dep_response {
u8 gt[];
} __packed;
+struct pn532_autopoll_resp {
+ u8 type;
+ u8 ln;
+ u8 tg;
+ u8 tgdata[];
+};
+
+/* PN532_CMD_IN_AUTOPOLL */
+#define PN532_AUTOPOLL_POLLNR_INFINITE 0xff
+#define PN532_AUTOPOLL_PERIOD 0x03 /* in units of 150 ms */
+
+#define PN532_AUTOPOLL_TYPE_GENERIC_106 0x00
+#define PN532_AUTOPOLL_TYPE_GENERIC_212 0x01
+#define PN532_AUTOPOLL_TYPE_GENERIC_424 0x02
+#define PN532_AUTOPOLL_TYPE_JEWEL 0x04
+#define PN532_AUTOPOLL_TYPE_MIFARE 0x10
+#define PN532_AUTOPOLL_TYPE_FELICA212 0x11
+#define PN532_AUTOPOLL_TYPE_FELICA424 0x12
+#define PN532_AUTOPOLL_TYPE_ISOA 0x20
+#define PN532_AUTOPOLL_TYPE_ISOB 0x23
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_106 0x40
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_212 0x41
+#define PN532_AUTOPOLL_TYPE_DEP_PASSIVE_424 0x42
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_106 0x80
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_212 0x81
+#define PN532_AUTOPOLL_TYPE_DEP_ACTIVE_424 0x82
/* PN533_TG_INIT_AS_TARGET */
#define PN533_INIT_TARGET_PASSIVE 0x1
@@ -1389,6 +1415,101 @@ static int pn533_poll_dep(struct nfc_dev *nfc_dev)
return rc;
}
+static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
+ struct sk_buff *resp)
+{
+ struct pn532_autopoll_resp *apr;
+ struct nfc_target nfc_tgt;
+ u8 nbtg;
+ int rc;
+
+ if (IS_ERR(resp)) {
+ rc = PTR_ERR(resp);
+
+ nfc_err(dev->dev, "%s autopoll complete error %d\n",
+ __func__, rc);
+
+ if (rc == -ENOENT) {
+ if (dev->poll_mod_count != 0)
+ return rc;
+ goto stop_poll;
+ } else if (rc < 0) {
+ nfc_err(dev->dev,
+ "Error %d when running autopoll\n", rc);
+ goto stop_poll;
+ }
+ }
+
+ nbtg = resp->data[0];
+ if ((nbtg > 2) || (nbtg <= 0))
+ return -EAGAIN;
+
+ apr = (struct pn532_autopoll_resp *)&resp->data[1];
+ while (nbtg--) {
+ memset(&nfc_tgt, 0, sizeof(struct nfc_target));
+ switch (apr->type) {
+ case PN532_AUTOPOLL_TYPE_ISOA:
+ dev_dbg(dev->dev, "ISOA\n");
+ rc = pn533_target_found_type_a(&nfc_tgt, apr->tgdata,
+ apr->ln - 1);
+ break;
+ case PN532_AUTOPOLL_TYPE_FELICA212:
+ case PN532_AUTOPOLL_TYPE_FELICA424:
+ dev_dbg(dev->dev, "FELICA\n");
+ rc = pn533_target_found_felica(&nfc_tgt, apr->tgdata,
+ apr->ln - 1);
+ break;
+ case PN532_AUTOPOLL_TYPE_JEWEL:
+ dev_dbg(dev->dev, "JEWEL\n");
+ rc = pn533_target_found_jewel(&nfc_tgt, apr->tgdata,
+ apr->ln - 1);
+ break;
+ case PN532_AUTOPOLL_TYPE_ISOB:
+ dev_dbg(dev->dev, "ISOB\n");
+ rc = pn533_target_found_type_b(&nfc_tgt, apr->tgdata,
+ apr->ln - 1);
+ break;
+ case PN532_AUTOPOLL_TYPE_MIFARE:
+ dev_dbg(dev->dev, "Mifare\n");
+ rc = pn533_target_found_type_a(&nfc_tgt, apr->tgdata,
+ apr->ln - 1);
+ break;
+ default:
+ nfc_err(dev->dev,
+ "Unknown current poll modulation\n");
+ rc = -EPROTO;
+ }
+
+ if (rc)
+ goto done;
+
+ if (!(nfc_tgt.supported_protocols & dev->poll_protocols)) {
+ nfc_err(dev->dev,
+ "The Tg found doesn't have the desired protocol\n");
+ rc = -EAGAIN;
+ goto done;
+ }
+
+ dev->tgt_available_prots = nfc_tgt.supported_protocols;
+ apr = (struct pn532_autopoll_resp *)
+ (apr->tgdata + (apr->ln - 1));
+ }
+
+ pn533_poll_reset_mod_list(dev);
+ nfc_targets_found(dev->nfc_dev, &nfc_tgt, 1);
+
+done:
+ dev_kfree_skb(resp);
+ return rc;
+
+stop_poll:
+ nfc_err(dev->dev, "autopoll operation has been stopped\n");
+
+ pn533_poll_reset_mod_list(dev);
+ dev->poll_protocols = 0;
+ return rc;
+}
+
static int pn533_poll_complete(struct pn533 *dev, void *arg,
struct sk_buff *resp)
{
@@ -1532,6 +1653,7 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev,
{
struct pn533 *dev = nfc_get_drvdata(nfc_dev);
struct pn533_poll_modulations *cur_mod;
+ struct sk_buff *skb;
u8 rand_mod;
int rc;
@@ -1557,9 +1679,73 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev,
tm_protocols = 0;
}
- pn533_poll_create_mod_list(dev, im_protocols, tm_protocols);
dev->poll_protocols = im_protocols;
dev->listen_protocols = tm_protocols;
+ if (dev->device_type == PN533_DEVICE_PN532_AUTOPOLL) {
+ skb = pn533_alloc_skb(dev, 4 + 6);
+ if (!skb)
+ return -ENOMEM;
+
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_POLLNR_INFINITE;
+ *((u8 *)skb_put(skb, sizeof(u8))) = PN532_AUTOPOLL_PERIOD;
+
+ if ((im_protocols & NFC_PROTO_MIFARE_MASK) &&
+ (im_protocols & NFC_PROTO_ISO14443_MASK) &&
+ (im_protocols & NFC_PROTO_NFC_DEP_MASK))
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_GENERIC_106;
+ else {
+ if (im_protocols & NFC_PROTO_MIFARE_MASK)
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_MIFARE;
+
+ if (im_protocols & NFC_PROTO_ISO14443_MASK)
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_ISOA;
+
+ if (im_protocols & NFC_PROTO_NFC_DEP_MASK) {
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_DEP_PASSIVE_106;
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_DEP_PASSIVE_212;
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_DEP_PASSIVE_424;
+ }
+ }
+
+ if (im_protocols & NFC_PROTO_FELICA_MASK ||
+ im_protocols & NFC_PROTO_NFC_DEP_MASK) {
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_FELICA212;
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_FELICA424;
+ }
+
+ if (im_protocols & NFC_PROTO_JEWEL_MASK)
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_JEWEL;
+
+ if (im_protocols & NFC_PROTO_ISO14443_B_MASK)
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_ISOB;
+
+ if (tm_protocols)
+ *((u8 *)skb_put(skb, sizeof(u8))) =
+ PN532_AUTOPOLL_TYPE_DEP_ACTIVE_106;
+
+ rc = pn533_send_cmd_async(dev, PN533_CMD_IN_AUTOPOLL, skb,
+ pn533_autopoll_complete, NULL);
+
+ if (rc < 0)
+ dev_kfree_skb(skb);
+ else
+ dev->poll_mod_count++;
+
+ return rc;
+ }
+
+ pn533_poll_create_mod_list(dev, im_protocols, tm_protocols);
/* Do not always start polling from the same modulation */
get_random_bytes(&rand_mod, sizeof(rand_mod));
@@ -2461,7 +2647,8 @@ static int pn533_dev_up(struct nfc_dev *nfc_dev)
if (dev->phy_ops->dev_up)
dev->phy_ops->dev_up(dev);
- if (dev->device_type == PN533_DEVICE_PN532) {
+ if ((dev->device_type == PN533_DEVICE_PN532) ||
+ (dev->device_type == PN533_DEVICE_PN532_AUTOPOLL)) {
int rc = pn532_sam_configuration(nfc_dev);
if (rc)
@@ -2508,6 +2695,7 @@ static int pn533_setup(struct pn533 *dev)
case PN533_DEVICE_PASORI:
case PN533_DEVICE_ACR122U:
case PN533_DEVICE_PN532:
+ case PN533_DEVICE_PN532_AUTOPOLL:
max_retries.mx_rty_atr = 0x2;
max_retries.mx_rty_psl = 0x1;
max_retries.mx_rty_passive_act =
@@ -2544,6 +2732,7 @@ static int pn533_setup(struct pn533 *dev)
switch (dev->device_type) {
case PN533_DEVICE_STD:
case PN533_DEVICE_PN532:
+ case PN533_DEVICE_PN532_AUTOPOLL:
break;
case PN533_DEVICE_PASORI:
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 6541088fad73..388fc1b4fcc1 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -6,10 +6,11 @@
* Copyright (C) 2012-2013 Tieto Poland
*/
-#define PN533_DEVICE_STD 0x1
-#define PN533_DEVICE_PASORI 0x2
-#define PN533_DEVICE_ACR122U 0x3
-#define PN533_DEVICE_PN532 0x4
+#define PN533_DEVICE_STD 0x1
+#define PN533_DEVICE_PASORI 0x2
+#define PN533_DEVICE_ACR122U 0x3
+#define PN533_DEVICE_PN532 0x4
+#define PN533_DEVICE_PN532_AUTOPOLL 0x5
#define PN533_ALL_PROTOCOLS (NFC_PROTO_JEWEL_MASK | NFC_PROTO_MIFARE_MASK |\
NFC_PROTO_FELICA_MASK | NFC_PROTO_ISO14443_MASK |\
@@ -75,6 +76,7 @@
#define PN533_CMD_IN_ATR 0x50
#define PN533_CMD_IN_RELEASE 0x52
#define PN533_CMD_IN_JUMP_FOR_DEP 0x56
+#define PN533_CMD_IN_AUTOPOLL 0x60
#define PN533_CMD_TG_INIT_AS_TARGET 0x8c
#define PN533_CMD_TG_GET_DATA 0x86
--
2.23.0
^ permalink raw reply related
* [PATCH v8 7/7] nfc: pn532_uart: Make use of pn532 autopoll
From: Lars Poeschel @ 2019-09-19 9:16 UTC (permalink / raw)
To: GitAuthor: Lars Poeschel, open list:NFC SUBSYSTEM, open list; +Cc: Johan Hovold
In-Reply-To: <20190919091645.16439-1-poeschel@lemonage.de>
This switches the pn532 UART phy driver from manually polling to the new
autopoll mechanism.
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
---
Changes in v6:
- Rebased the patch series on v5.3-rc5
drivers/nfc/pn533/uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/pn533/uart.c b/drivers/nfc/pn533/uart.c
index 7f639051cdd0..edf8db890eaa 100644
--- a/drivers/nfc/pn533/uart.c
+++ b/drivers/nfc/pn533/uart.c
@@ -262,7 +262,7 @@ static int pn532_uart_probe(struct serdev_device *serdev)
serdev_device_set_flow_control(serdev, false);
pn532->send_wakeup = PN532_SEND_WAKEUP;
timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
- priv = pn53x_common_init(PN533_DEVICE_PN532,
+ priv = pn53x_common_init(PN533_DEVICE_PN532_AUTOPOLL,
PN533_PROTO_REQ_ACK_RESP,
pn532, &uart_phy_ops, NULL,
&pn532->serdev->dev);
--
2.23.0
^ permalink raw reply related
* Re: [PATCH RFC v3 2/5] net: Add NETIF_F_GRO_LIST feature
From: Steffen Klassert @ 2019-09-19 9:24 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Paolo Abeni
In-Reply-To: <CA+FuTSeBmGY4_2X3Ydhf60G=An9g9iikDBQMDji=XptN_jBqiw@mail.gmail.com>
On Wed, Sep 18, 2019 at 12:10:31PM -0400, Willem de Bruijn wrote:
> On Wed, Sep 18, 2019 at 3:25 AM Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> >
> > This adds a new NETIF_F_GRO_LIST feature flag. I will be used
> > to configure listfyed GRO what will be implemented with some
> > followup paches.
>
> This should probably simultaneously introduce SKB_GSO_FRAGLIST as well
> as a BUILD_BUG_ON in net_gso_ok.
Yes, good point. I'll also rename NETIF_F_GRO_LIST to NETIF_F_GRO_FRAGLIST
and add NETIF_F_GSO_FRAGLIST what is currently missing.
>
> Please also in the commit describe the constraints of skbs that have
> this type. If I'm not mistaken, an skb with either gso_size linear
> data or one gso_sized frag, followed by a frag_list of the same. With
> the exception of the last frag_list member, whose mss may be less than
> gso_size. This will help when reasoning about all the types of skbs we
> may see at segmentation, as we recently had to do [1]
We don't use skb_segment(), so I think we don't have this constraint.
>
> Minor nit: I think it's listified, not listifyed.
I think neither of both words really exist :)
I'll rename it to fraglist GRO.
^ permalink raw reply
* [PATCH -net] staging: octeon: se "(uintptr_t)" to cast from pointer to int
From: Geert Uytterhoeven @ 2019-09-19 9:25 UTC (permalink / raw)
To: Matthew Wilcox, David S . Miller
Cc: Greg Kroah-Hartman, netdev, devel, linux-kernel,
Geert Uytterhoeven
On 32-bit:
In file included from drivers/staging/octeon/octeon-ethernet.h:41,
from drivers/staging/octeon/ethernet-tx.c:25:
drivers/staging/octeon/octeon-stubs.h: In function ‘cvmx_phys_to_ptr’:
drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
return (void *)(physical_address);
^
drivers/staging/octeon/ethernet-tx.c: In function ‘cvm_oct_xmit’:
drivers/staging/octeon/ethernet-tx.c:264:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:268:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:276:20: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
XKPHYS_TO_PHYS((u64)skb_frag_address(fs));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:280:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)CVM_OCT_SKB_CB(skb));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
Fix this by replacing casts to "u64" by casts to "uintptr_t", which is
either 32-bit or 64-bit, and adding an intermediate cast to "uintptr_t"
where needed.
Exposed by commit 171a9bae68c72f2d ("staging/octeon: Allow test build on
!MIPS").
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/staging/octeon/ethernet-tx.c | 9 +++++----
drivers/staging/octeon/octeon-stubs.h | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index c64728fc21f229d8..7021ff07ba2a0b70 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -261,11 +261,11 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
/* Build the PKO buffer pointer */
hw_buffer.u64 = 0;
if (skb_shinfo(skb)->nr_frags == 0) {
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
+ hw_buffer.s.addr = XKPHYS_TO_PHYS((uintptr_t)skb->data);
hw_buffer.s.pool = 0;
hw_buffer.s.size = skb->len;
} else {
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
+ hw_buffer.s.addr = XKPHYS_TO_PHYS((uintptr_t)skb->data);
hw_buffer.s.pool = 0;
hw_buffer.s.size = skb_headlen(skb);
CVM_OCT_SKB_CB(skb)[0] = hw_buffer.u64;
@@ -273,11 +273,12 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
skb_frag_t *fs = skb_shinfo(skb)->frags + i;
hw_buffer.s.addr =
- XKPHYS_TO_PHYS((u64)skb_frag_address(fs));
+ XKPHYS_TO_PHYS((uintptr_t)skb_frag_address(fs));
hw_buffer.s.size = skb_frag_size(fs);
CVM_OCT_SKB_CB(skb)[i + 1] = hw_buffer.u64;
}
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)CVM_OCT_SKB_CB(skb));
+ hw_buffer.s.addr =
+ XKPHYS_TO_PHYS((uintptr_t)CVM_OCT_SKB_CB(skb));
hw_buffer.s.size = skb_shinfo(skb)->nr_frags + 1;
pko_command.s.segs = skb_shinfo(skb)->nr_frags + 1;
pko_command.s.gather = 1;
diff --git a/drivers/staging/octeon/octeon-stubs.h b/drivers/staging/octeon/octeon-stubs.h
index a4ac3bfb62a85e65..b78ce9eaab85d310 100644
--- a/drivers/staging/octeon/octeon-stubs.h
+++ b/drivers/staging/octeon/octeon-stubs.h
@@ -1202,7 +1202,7 @@ static inline int cvmx_wqe_get_grp(cvmx_wqe_t *work)
static inline void *cvmx_phys_to_ptr(uint64_t physical_address)
{
- return (void *)(physical_address);
+ return (void *)(uintptr_t)(physical_address);
}
static inline uint64_t cvmx_ptr_to_phys(void *ptr)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH RFC v3 2/5] net: Add NETIF_F_GRO_LIST feature
From: Steffen Klassert @ 2019-09-19 9:32 UTC (permalink / raw)
To: Subash Abhinov Kasiviswanathan
Cc: Willem de Bruijn, Network Development, Paolo Abeni
In-Reply-To: <8bc2e1658e74963d6c3ff297acdcbce6@codeaurora.org>
On Wed, Sep 18, 2019 at 08:04:18PM -0600, Subash Abhinov Kasiviswanathan wrote:
> On 2019-09-18 10:10, Willem de Bruijn wrote:
> > On Wed, Sep 18, 2019 at 3:25 AM Steffen Klassert
> > <steffen.klassert@secunet.com> wrote:
> > >
> > > This adds a new NETIF_F_GRO_LIST feature flag. I will be used
> > > to configure listfyed GRO what will be implemented with some
> > > followup paches.
> >
> > This should probably simultaneously introduce SKB_GSO_FRAGLIST as well
> > as a BUILD_BUG_ON in net_gso_ok.
> >
> > Please also in the commit describe the constraints of skbs that have
> > this type. If I'm not mistaken, an skb with either gso_size linear
> > data or one gso_sized frag, followed by a frag_list of the same. With
> > the exception of the last frag_list member, whose mss may be less than
> > gso_size. This will help when reasoning about all the types of skbs we
> > may see at segmentation, as we recently had to do [1]
> >
>
> Would it be preferrable to allow any size skbs for the listification.
We currently require a single gso_size because we adjust uh->len
on the head skb to the full size to do correct memory accounting
on the local input path. That is going to be restored with the
gso_size on segmentation.
> Since the original skbs are being restored, single gso_size shoudln't
> be a constraint here.
It might be possible to allow any sized skbs with some extra work, though.
^ permalink raw reply
* Re: [PATCH RFC v3 5/5] udp: Support UDP fraglist GRO/GSO.
From: Steffen Klassert @ 2019-09-19 9:33 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Paolo Abeni
In-Reply-To: <CA+FuTSfSpzHCCpDRD2s0fP3u2oyKVKQPOVAp0LLPMPV+W3kFtw@mail.gmail.com>
On Wed, Sep 18, 2019 at 12:13:26PM -0400, Willem de Bruijn wrote:
> On Wed, Sep 18, 2019 at 3:25 AM Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> >
> > This patch extends UDP GRO to support fraglist GRO/GSO
> > by using the previously introduced infrastructure.
> > All UDP packets that are not targeted to a GRO capable
> > UDP sockets are going to fraglist GRO now (local input
> > and forward).
> >
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>
>
> > @@ -419,7 +460,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> > (skb->ip_summed != CHECKSUM_PARTIAL &&
> > NAPI_GRO_CB(skb)->csum_cnt == 0 &&
> > !NAPI_GRO_CB(skb)->csum_valid))
> > - goto out_unlock;
> > + goto out;
> >
> > /* mark that this skb passed once through the tunnel gro layer */
> > NAPI_GRO_CB(skb)->encap_mark = 1;
> > @@ -446,8 +487,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
> > skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> > pp = call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
> >
> > -out_unlock:
> > - rcu_read_unlock();
> > +out:
> > skb_gro_flush_final(skb, pp, flush);
> > return pp;
> > }
>
> This probably belongs in patch 1?
Yes, apparently :)
^ permalink raw reply
* Re: [PATCH RFC v3 0/5] Support fraglist GRO/GSO
From: Steffen Klassert @ 2019-09-19 9:41 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Paolo Abeni, marcelo.leitner
In-Reply-To: <CA+FuTSdVFguDHXYPJBRrLhzPWBaykd+7PRqEmGf_eOFC3iHpAg@mail.gmail.com>
On Wed, Sep 18, 2019 at 12:17:08PM -0400, Willem de Bruijn wrote:
> On Wed, Sep 18, 2019 at 3:25 AM Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> >
> > This patchset adds support to do GRO/GSO by chaining packets
> > of the same flow at the SKB frag_list pointer. This avoids
> > the overhead to merge payloads into one big packet, and
> > on the other end, if GSO is needed it avoids the overhead
> > of splitting the big packet back to the native form.
> >
> > Patch 1 Enables UDP GRO by default.
> >
> > Patch 2 adds a netdev feature flag to enable listifyed GRO,
> > this implements one of the configuration options discussed
> > at netconf 2019.
> >
> > Patch 3 adds a netdev software feature set that defaults to off
> > and assigns the new listifyed GRO feature flag to it.
> >
> > Patch 4 adds the core infrastructure to do fraglist GRO/GSO.
> >
> > Patch 5 enables UDP to use fraglist GRO/GSO if configured and no
> > GRO supported socket is found.
>
> Very nice feature, Steffen.
Thanks!
> Aside from questions around performance,
> my only question is really how this relates to GSO_BY_FRAGS.
>
> More specifically, whether we can remove that in favor of using your
> new skb_segment_list. That would actually be a big first step in
> simplifying skb_segment back to something manageable.
As Marcelo pointed out, this should be doable.
Thanks for all your review. I'll incorporate your comments and do
RFC v4, so that we hopefully can start the mainlining process as
soon as net-next opens again.
^ permalink raw reply
* RE: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
From: Jianyong Wu (Arm Technology China) @ 2019-09-19 9:46 UTC (permalink / raw)
To: Paolo Bonzini, netdev@vger.kernel.org, yangbo.lu@nxp.com,
john.stultz@linaro.org, tglx@linutronix.de,
sean.j.christopherson@intel.com, maz@kernel.org,
richardcochran@gmail.com, Mark Rutland, Will Deacon,
Suzuki Poulose
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Steve Capper,
Kaly Xin (Arm Technology China), Justin He (Arm Technology China),
nd, linux-arm-kernel@lists.infradead.org
In-Reply-To: <629538ea-13fb-e666-8df6-8ad23f114755@redhat.com>
Hi Paolo,
> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Wednesday, September 18, 2019 6:24 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will
> Deacon <Will.Deacon@arm.com>; Suzuki Poulose
> <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
>
> On 18/09/19 11:57, Jianyong Wu (Arm Technology China) wrote:
> > Hi Paolo,
> >
> >> On 18/09/19 10:07, Jianyong Wu wrote:
> >>> + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> >>> + getnstimeofday(ts);
> >>
> >> This is not Y2038-safe. Please use ktime_get_real_ts64 instead, and
> >> split the 64-bit seconds value between val[0] and val[1].
> >>
> > As far as I know, y2038-safe will only affect signed 32-bit integer,
> > how does it affect 64-bit integer?
> > And why split 64-bit number into two blocks is necessary?
>
> val is an u32, not an u64. (And val[0], where you store the seconds, is best
> treated as signed, since val[0] == -1 is returned for
> SMCCC_RET_NOT_SUPPORTED).
>
Yeah, need consider twice.
Val[] should be long not u32 I think, so in arm64 I can avoid that Y2038_safe, but
also need rewrite for arm32.
> >> However, it seems to me that the new function is not needed and you
> >> can just use ktime_get_snapshot. You'll get the time in
> >> systime_snapshot->real and the cycles value in systime_snapshot->cycles.
> >
> > See patch 5/6, I need both counter cycle and clocksource,
> ktime_get_snapshot seems only offer cycles.
>
> No, patch 5/6 only needs the current clock (ptp_sc.cycles is never accessed).
> So you could just use READ_ONCE(tk->tkr_mono.clock).
>
Yeah, patch 5/6 just need clocksource, but I think tk->tkr_mono.clock can't read in external like module,
So I need an API to expose clocksource.
> However, even then I don't think it is correct to use ptp_sc.cs blindly in patch
> 5. I think there is a misunderstanding on the meaning of
> system_counterval.cs as passed to get_device_system_crosststamp.
> system_counterval.cs is not the active clocksource; it's the clocksource on
> which system_counterval.cycles is based.
>
I think we can use system_counterval_t as pass current clocksource to system_counterval_t.cs and its
corresponding cycles to system_counterval_t.cycles. is it a big problem?
> Hypothetically, the clocksource could be one for which ptp_sc.cycles is _not_
> a cycle value. If you set system_counterval.cs to the system clocksource,
> get_device_system_crosststamp will return a bogus value.
Yeah, but in patch 3/6, we have a corresponding pair of clock source and cycle value. So I think there will be no
that problem in this patch set.
In the implementation of get_device_system_crosststamp:
"
...
if (tk->tkr_mono.clock != system_counterval.cs)
return -ENODEV;
...
"
We need tk->tkr_mono.clock passed to get_device_system_crosststamp, just like patch 3/6 do, otherwise will return error.
> So system_counterval.cs should be set to something like
> &clocksource_counter (from drivers/clocksource/arm_arch_timer.c).
> Perhaps the right place to define kvm_arch_ptp_get_clock_fn is in that file?
>
I have checked that ptp_sc.cs is arch_sys_counter.
Also move the module API to arm_arch_timer.c will looks a little ugly and it's not easy to be accept by arm side I think.
> >>> + get_current_counterval(&sc);
> >>> + val[0] = ts->tv_sec;
> >>> + val[1] = ts->tv_nsec;
> >>> + val[2] = sc.cycles;
> >>> + val[3] = 0;
> >>> + break;
> >>
> >> This should return a guest-cycles value. If the cycles values always
> >> the same between the host and the guest on ARM, then okay. If not,
> >> you have to apply whatever offset exists.
> >>
> > In my opinion, when use ptp_kvm as clock sources to sync time between
> > host and guest, user should promise the guest and host has no clock
> > offset.
>
> What would be the adverse effect of having a fixed offset between guest
> and host? If there were one, you'd have to check that and fail the hypercall if
> there is an offset. But again, I think it's enough to subtract
> vcpu_vtimer(vcpu)->cntvoff or something like that.
>
Sure, counter offset should be considered.
> You also have to check here that the clocksource is based on the ARM
> architectural timer. Again, maybe you could place the implementation in
> drivers/clocksource/arm_arch_timer.c, and make it return -ENODEV if the
> active clocksource is not clocksource_counter. Then KVM can look for errors
> and return SMCCC_RET_NOT_SUPPORTED in that case.
I have checked it. The clock source is arch_sys_counter which is arm arch timer.
I can try to do that but I'm not sure arm side will be happy for that change.
Thanks
Jianyong Wu
>
> Thanks,
>
> Paolo
>
> > So we can be sure that the cycle between guest and host should be keep
> > consistent. But I need check it.
> > I think host cycle should be returned to guest as we should promise we
> > get clock and counter in the same time.
^ permalink raw reply
* [PATCH v2] staging: octeon: Use "(uintptr_t)" to cast from pointer to int
From: Geert Uytterhoeven @ 2019-09-19 9:50 UTC (permalink / raw)
To: Matthew Wilcox, David S . Miller
Cc: Greg Kroah-Hartman, netdev, devel, linux-kernel,
Geert Uytterhoeven
On 32-bit:
In file included from drivers/staging/octeon/octeon-ethernet.h:41,
from drivers/staging/octeon/ethernet-tx.c:25:
drivers/staging/octeon/octeon-stubs.h: In function ‘cvmx_phys_to_ptr’:
drivers/staging/octeon/octeon-stubs.h:1205:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
return (void *)(physical_address);
^
drivers/staging/octeon/ethernet-tx.c: In function ‘cvm_oct_xmit’:
drivers/staging/octeon/ethernet-tx.c:264:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:268:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:276:20: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
XKPHYS_TO_PHYS((u64)skb_frag_address(fs));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
drivers/staging/octeon/ethernet-tx.c:280:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)CVM_OCT_SKB_CB(skb));
^
drivers/staging/octeon/octeon-stubs.h:2:30: note: in definition of macro ‘XKPHYS_TO_PHYS’
#define XKPHYS_TO_PHYS(p) (p)
^
Fix this by replacing casts to "u64" by casts to "uintptr_t", which is
either 32-bit or 64-bit, and adding an intermediate cast to "uintptr_t"
where needed.
Exposed by commit 171a9bae68c72f2d ("staging/octeon: Allow test build on
!MIPS").
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
v2:
- Fix silly typo in oneline-summary.
---
drivers/staging/octeon/ethernet-tx.c | 9 +++++----
drivers/staging/octeon/octeon-stubs.h | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index c64728fc21f229d8..7021ff07ba2a0b70 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -261,11 +261,11 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
/* Build the PKO buffer pointer */
hw_buffer.u64 = 0;
if (skb_shinfo(skb)->nr_frags == 0) {
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
+ hw_buffer.s.addr = XKPHYS_TO_PHYS((uintptr_t)skb->data);
hw_buffer.s.pool = 0;
hw_buffer.s.size = skb->len;
} else {
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)skb->data);
+ hw_buffer.s.addr = XKPHYS_TO_PHYS((uintptr_t)skb->data);
hw_buffer.s.pool = 0;
hw_buffer.s.size = skb_headlen(skb);
CVM_OCT_SKB_CB(skb)[0] = hw_buffer.u64;
@@ -273,11 +273,12 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
skb_frag_t *fs = skb_shinfo(skb)->frags + i;
hw_buffer.s.addr =
- XKPHYS_TO_PHYS((u64)skb_frag_address(fs));
+ XKPHYS_TO_PHYS((uintptr_t)skb_frag_address(fs));
hw_buffer.s.size = skb_frag_size(fs);
CVM_OCT_SKB_CB(skb)[i + 1] = hw_buffer.u64;
}
- hw_buffer.s.addr = XKPHYS_TO_PHYS((u64)CVM_OCT_SKB_CB(skb));
+ hw_buffer.s.addr =
+ XKPHYS_TO_PHYS((uintptr_t)CVM_OCT_SKB_CB(skb));
hw_buffer.s.size = skb_shinfo(skb)->nr_frags + 1;
pko_command.s.segs = skb_shinfo(skb)->nr_frags + 1;
pko_command.s.gather = 1;
diff --git a/drivers/staging/octeon/octeon-stubs.h b/drivers/staging/octeon/octeon-stubs.h
index a4ac3bfb62a85e65..b78ce9eaab85d310 100644
--- a/drivers/staging/octeon/octeon-stubs.h
+++ b/drivers/staging/octeon/octeon-stubs.h
@@ -1202,7 +1202,7 @@ static inline int cvmx_wqe_get_grp(cvmx_wqe_t *work)
static inline void *cvmx_phys_to_ptr(uint64_t physical_address)
{
- return (void *)(physical_address);
+ return (void *)(uintptr_t)(physical_address);
}
static inline uint64_t cvmx_ptr_to_phys(void *ptr)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH -net] zd1211rw: zd_usb: Use "%zu" to format size_t
From: Kalle Valo @ 2019-09-19 9:50 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Gustavo A . R . Silva, Daniel Drake, Ulrich Kunitz,
David S . Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190919091532.24951-1-geert@linux-m68k.org>
Geert Uytterhoeven <geert@linux-m68k.org> writes:
> On 32-bit:
>
> drivers/net/wireless/zydas/zd1211rw/zd_usb.c: In function ‘check_read_regs’:
> drivers/net/wireless/zydas/zd1211rw/zd_def.h:18:25: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
> dev_printk(level, dev, "%s() " fmt, __func__, ##args)
> ^~~~~~~
> drivers/net/wireless/zydas/zd1211rw/zd_def.h:22:4: note: in expansion of macro ‘dev_printk_f’
> dev_printk_f(KERN_DEBUG, dev, fmt, ## args)
> ^~~~~~~~~~~~
> drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1635:3: note: in expansion of macro ‘dev_dbg_f’
> dev_dbg_f(zd_usb_dev(usb),
> ^~~~~~~~~
> drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1636:51: note: format string is defined here
> "error: actual length %d less than expected %ld\n",
> ~~^
> %d
>
> Fixes: 84b0b66352470e64 ("zd1211rw: zd_usb: Use struct_size() helper")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
I'll queue this for v5.4.
--
Kalle Valo
^ permalink raw reply
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race
From: Алексей Захаров @ 2019-09-19 9:53 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev
In-Reply-To: <9357.1568880036@nyx>
чт, 19 сент. 2019 г. в 11:00, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Алексей Захаров wrote:
>
> >> >Once a while, one of 802.3ad slaves fails to initialize and hangs in
> >> >BOND_LINK_FAIL state. Commit 334031219a84 ("bonding/802.3ad: fix slave
> >> >link initialization transition states") checks slave->last_link_up. But
> >> >link can still hang in weird state.
> >> >After physical link comes up it sends first two LACPDU messages and
> >> >doesn't work properly after that. It doesn't send or receive LACPDU.
> >> >Once it happens, the only message in dmesg is:
> >> >bond1: link status up again after 0 ms for interface eth2
> >>
> >> I believe this message indicates that the slave entered
> >> BOND_LINK_FAIL state, but downdelay was not set. The _FAIL state is
> >> really for managing the downdelay expiration, and a slave should not be
> >> in that state (outside of a brief transition entirely within
> >> bond_miimon_inspect) if downdelay is 0.
> >That's true, downdelay was set to 0, we only use updelay 500.
> >Does it mean, that the bonding driver shouldn't set slave to FAIL
> >state in this case?
>
> It really shouldn't change the slave->link outside of the
> monitoring functions at all, because there are side effects that are not
> happening (user space notifications, updelay / downdelay, etc).
>
> >> >This behavior can be reproduced (not every time):
> >> >1. Set slave link down
> >> >2. Wait for 1-3 seconds
> >> >3. Set slave link up
> >> >
> >> >The fix is to check slave->link before setting it to BOND_LINK_FAIL or
> >> >BOND_LINK_DOWN state. If got invalid Speed/Dupex values and link is in
> >> >BOND_LINK_UP state, mark it as BOND_LINK_FAIL; otherwise mark it as
> >> >BOND_LINK_DOWN.
> >> >
> >> >Fixes: 334031219a84 ("bonding/802.3ad: fix slave link initialization
> >> >transition states")
> >> >Signed-off-by: Aleksei Zakharov <zakharov.a.g@yandex.ru>
> >> >---
> >> > drivers/net/bonding/bond_main.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> >index 931d9d935686..a28776d8f33f 100644
> >> >--- a/drivers/net/bonding/bond_main.c
> >> >+++ b/drivers/net/bonding/bond_main.c
> >> >@@ -3135,7 +3135,7 @@ static int bond_slave_netdev_event(unsigned long event,
> >> > */
> >> > if (bond_update_speed_duplex(slave) &&
> >> > BOND_MODE(bond) == BOND_MODE_8023AD) {
> >> >- if (slave->last_link_up)
> >> >+ if (slave->link == BOND_LINK_UP)
> >> > slave->link = BOND_LINK_FAIL;
> >> > else
> >> > slave->link = BOND_LINK_DOWN;
> >>
> >> Is the core problem here that slaves are reporting link up, but
> >> returning invalid values for speed and/or duplex? If so, what network
> >> device are you testing with that is exhibiting this behavior?
> >That's true, because link becomes FAIL right in this block of code.
> >We use Mellanox ConnectX-3 Pro nic.
> >
> >>
> >> If I'm not mistaken, there have been several iterations of
> >> hackery on this block of code to work around this same problem, and each
> >> time there's some corner case that still doesn't work.
> >As i can see, commit 4d2c0cda0744 ("bonding: speed/duplex update at
> >NETDEV_UP event")
> >introduced BOND_LINK_DOWN state if update speed/duplex failed.
> >
> >Commit ea53abfab960 ("bonding/802.3ad: fix link_failure_count tracking")
> >changed DOWN state to FAIL.
> >
> >Commit 334031219a84 ("bonding/802.3ad: fix slave link initialization
> >transition states")
> >implemented different new state for different current states, but it
> >was based on slave->last_link_up.
> >In our case slave->last_link_up !=0 when this code runs. But, slave is
> >not in UP state at the moment. It becomes
> >FAIL and hangs in this state.
> >So, it looks like checking if slave is in UP mode is more appropriate
> >here. At least it works in our case.
> >
> >There was one more commit 12185dfe4436 ("bonding: Force slave speed
> >check after link state recovery for 802.3ad")
> >but it doesn't help in our case.
> >
> >>
> >> As Davem asked last time around, is the real problem that device
> >> drivers report carrier up but supply invalid speed and duplex state?
> >Probably, but I'm not quite sure right now. We didn't face this issue
> >before 4d2c0cda0744 and ea53abfab960
> >commits.
>
> My concern here is that we keep adding special cases to this
> code apparently without really understanding the root cause of the
> failures. 4d2c0cda0744 asserts that there is a problem that drivers are
> not supplying speed and duplex information at NETDEV_UP time, but is not
> specific as to the details (hardware information). Before we add
> another change, I would like to understand what the actual underlying
> cause of the failure is, and if yours is somehow different from what
> 4d2c0cda0744 or ea53abfab960 were fixing (or trying to fix).
>
> Would it be possible for you to instrument the code here to dump
> out the duplex/speed failure information and carrier state of the slave
> device at this point when it fails in your testing? Something like the
> following (which I have not compile tested):
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 931d9d935686..758af8c2b9e1 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -378,15 +378,22 @@ static int bond_update_speed_duplex(struct slave *slave)
> slave->duplex = DUPLEX_UNKNOWN;
>
> res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
> - if (res < 0)
> + if (res < 0) {
> + pr_err("DBG ksettings res %d slave %s\n", res, slave_dev->name);
> return 1;
> - if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
> + }
> + if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1)) {
> + pr_err("DBG speed %u slave %s\n", ecmd.base.speed,
> + slave_dev->name);
> return 1;
> + }
> switch (ecmd.base.duplex) {
> case DUPLEX_FULL:
> case DUPLEX_HALF:
> break;
> default:
> + pr_err("DBG duplex %u slave %s\n", ecmd.base.duplex,
> + slave_dev->name);
> return 1;
> }
>
> @@ -3135,6 +3142,9 @@ static int bond_slave_netdev_event(unsigned long event,
> */
> if (bond_update_speed_duplex(slave) &&
> BOND_MODE(bond) == BOND_MODE_8023AD) {
> + pr_err("DBG slave %s event %d carrier %d\n",
> + slave->dev->name, event,
> + netif_carrier_ok(slave->dev));
> if (slave->last_link_up)
> slave->link = BOND_LINK_FAIL;
> else
Thanks, did that, without my patch. Here is the output when link doesn't work.
Host has actor port state 71 and partner port state 1:
[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Steering Mode 1
[Thu Sep 19 12:14:04 2019] DBG speed 4294967295 slave eth2
[Thu Sep 19 12:14:04 2019] DBG slave eth2 event 1 carrier 0
[Thu Sep 19 12:14:04 2019] 8021q: adding VLAN 0 to HW filter on device eth2
[Thu Sep 19 12:14:04 2019] mlx4_en: eth2: Link Up
[Thu Sep 19 12:14:04 2019] bond-san: link status up again after 0 ms
for interface eth2
Here is the output when everything works fine:
[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Steering Mode 1
[Thu Sep 19 12:15:40 2019] DBG speed 4294967295 slave eth2
[Thu Sep 19 12:15:40 2019] DBG slave eth2 event 1 carrier 0
[Thu Sep 19 12:15:40 2019] 8021q: adding VLAN 0 to HW filter on device eth2
[Thu Sep 19 12:15:40 2019] bond-san: link status definitely down for
interface eth2, disabling it
[Thu Sep 19 12:15:40 2019] mlx4_en: eth2: Link Up
[Thu Sep 19 12:15:40 2019] bond-san: link status up for interface
eth2, enabling it in 500 ms
[Thu Sep 19 12:15:41 2019] bond-san: link status definitely up for
interface eth2, 10000 Mbps full duplex
If I'm not mistaken, there's up event before carrier is up.
--
Best Regards,
Aleksei Zakharov
System administrator
^ permalink raw reply
* [PATCH v1 2/3] seccomp: avoid overflow in implicit constant conversion
From: Christian Brauner @ 2019-09-19 9:59 UTC (permalink / raw)
To: keescook, luto
Cc: jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Christian Brauner,
Tyler Hicks, Tycho Andersen, stable
In-Reply-To: <20190919095903.19370-1-christian.brauner@ubuntu.com>
USER_NOTIF_MAGIC is assigned to int variables in this test so set it to INT_MAX
to avoid warnings:
seccomp_bpf.c: In function ‘user_notification_continue’:
seccomp_bpf.c:3088:26: warning: overflow in implicit constant conversion [-Woverflow]
#define USER_NOTIF_MAGIC 116983961184613L
^
seccomp_bpf.c:3572:15: note: in expansion of macro ‘USER_NOTIF_MAGIC’
resp.error = USER_NOTIF_MAGIC;
^~~~~~~~~~~~~~~~
Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Tycho Andersen <tycho@tycho.ws>
Cc: stable@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
---
/* v1 */
unchanged
/* v0 */
Link: https://lore.kernel.org/r/20190918084833.9369-4-christian.brauner@ubuntu.com
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 6ef7f16c4cf5..e996d7b7fd6e 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -35,6 +35,7 @@
#include <stdbool.h>
#include <string.h>
#include <time.h>
+#include <limits.h>
#include <linux/elf.h>
#include <sys/uio.h>
#include <sys/utsname.h>
@@ -3072,7 +3073,7 @@ static int user_trap_syscall(int nr, unsigned int flags)
return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
}
-#define USER_NOTIF_MAGIC 116983961184613L
+#define USER_NOTIF_MAGIC INT_MAX
TEST(user_notification_basic)
{
pid_t pid;
--
2.23.0
^ permalink raw reply related
* [PATCH v1 1/3] seccomp: add SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: Christian Brauner @ 2019-09-19 9:59 UTC (permalink / raw)
To: keescook, luto
Cc: jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Christian Brauner,
Tycho Andersen, Tyler Hicks
In-Reply-To: <20190919095903.19370-1-christian.brauner@ubuntu.com>
This allows the seccomp notifier to continue a syscall. A positive
discussion about this feature was triggered by a post to the
ksummit-discuss mailing list (cf. [3]) and took place during KSummit
(cf. [1]) and again at the containers/checkpoint-restore
micro-conference at Linux Plumbers.
Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
which enables a process (watchee) to retrieve an fd for its seccomp
filter. This fd can then be handed to another (usually more privileged)
process (watcher). The watcher will then be able to receive seccomp
messages about the syscalls having been performed by the watchee.
This feature is heavily used in some userspace workloads. For example,
it is currently used to intercept mknod() syscalls in user namespaces
aka in containers.
The mknod() syscall can be easily filtered based on dev_t. This allows
us to only intercept a very specific subset of mknod() syscalls.
Furthermore, mknod() is not possible in user namespaces toto coelo and
so intercepting and denying syscalls that are not in the whitelist on
accident is not a big deal. The watchee won't notice a difference.
In contrast to mknod(), a lot of other syscall we intercept (e.g.
setxattr()) cannot be easily filtered like mknod() because they have
pointer arguments. Additionally, some of them might actually succeed in
user namespaces (e.g. setxattr() for all "user.*" xattrs). Since we
currently cannot tell seccomp to continue from a user notifier we are
stuck with performing all of the syscalls in lieu of the container. This
is a huge security liability since it is extremely difficult to
correctly assume all of the necessary privileges of the calling task
such that the syscall can be successfully emulated without escaping
other additional security restrictions (think missing CAP_MKNOD for
mknod(), or MS_NODEV on a filesystem etc.). This can be solved by
telling seccomp to resume the syscall.
One thing that came up in the discussion was the problem that another
thread could change the memory after userspace has decided to let the
syscall continue which is a well known TOCTOU with seccomp which is
present in other ways already.
The discussion showed that this feature is already very useful for any
syscall without pointer arguments. For any accidentally intercepted
non-pointer syscall it is safe to continue.
For syscalls with pointer arguments there is a race but for any cautious
userspace and the main usec cases the race doesn't matter. The notifier
is intended to be used in a scenario where a more privileged watcher
supervises the syscalls of lesser privileged watchee to allow it to get
around kernel-enforced limitations by performing the syscall for it
whenever deemed save by the watcher. Hence, if a user tricks the watcher
into allowing a syscall they will either get a deny based on
kernel-enforced restrictions later or they will have changed the
arguments in such a way that they manage to perform a syscall with
arguments that they would've been allowed to do anyway.
In general, it is good to point out again, that the notifier fd was not
intended to allow userspace to implement a security policy but rather to
work around kernel security mechanisms in cases where the watcher knows
that a given action is safe to perform.
/* References */
[1]: https://linuxplumbersconf.org/event/4/contributions/560
[2]: https://linuxplumbersconf.org/event/4/contributions/477
[3]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
[4]: commit 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
CC: Tyler Hicks <tyhicks@canonical.com>
---
/* v1 */
- Kees Cook <keescook@chromium.org>, Tycho Andersen <tycho@tycho.ws>:
- s/SECCOMP_RET_USER_NOTIF_ALLOW/SECCOMP_USER_NOTIF_FLAG_CONTINUE/g
- Kees Cook <keescook@chromium.org>:
- put giant warning about the dangers, and correct usage of the
SECCOMP_USER_NOTIF_FLAG_CONTINUE flag
- Kees Cook <keescook@chromium.org>:
- change return type for seccomp_do_user_notification() to int to align with
similar functions
/* v0 */
Link: https://lore.kernel.org/r/20190918084833.9369-2-christian.brauner@ubuntu.com
---
include/uapi/linux/seccomp.h | 20 ++++++++++++++++++++
kernel/seccomp.c | 28 ++++++++++++++++++++++------
2 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 90734aa5aa36..8a5dafed8a64 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -76,6 +76,26 @@ struct seccomp_notif {
struct seccomp_data data;
};
+/*
+ * Valid flags for struct seccomp_notif_resp
+ *
+ * Note, the SECCOMP_USER_NOTIF_FLAG_CONTINUE flag must be used with caution!
+ * If set by the process supervising the syscalls of another process the
+ * syscall will continue. This is problematic because of an inherent TOCTOU.
+ * An attacker can exploit the time while the supervised process is waiting on
+ * a response from the supervising process to rewrite syscall arguments which
+ * are passed as pointers of the intercepted syscall.
+ * It should be absolutely clear that this means that the seccomp notifier
+ * _cannot_ be used to implement a security policy! It should only ever be used
+ * in scenarios where a more privileged process supervises the syscalls of a
+ * lesser privileged process to get around kernel-enforced security
+ * restrictions when the privileged process deems this safe. In other words,
+ * in order to continue a syscall the supervising process should be sure that
+ * another security mechanism or the kernel itself will sufficiently block
+ * syscalls if arguments are rewritten to something unsafe.
+ */
+#define SECCOMP_USER_NOTIF_FLAG_CONTINUE BIT(0)
+
struct seccomp_notif_resp {
__u64 id;
__s64 val;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index dba52a7db5e8..12d2227e5786 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -75,6 +75,7 @@ struct seccomp_knotif {
/* The return values, only valid when in SECCOMP_NOTIFY_REPLIED */
int error;
long val;
+ u32 flags;
/* Signals when this has entered SECCOMP_NOTIFY_REPLIED */
struct completion ready;
@@ -732,11 +733,12 @@ static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
return filter->notif->next_id++;
}
-static void seccomp_do_user_notification(int this_syscall,
- struct seccomp_filter *match,
- const struct seccomp_data *sd)
+static int seccomp_do_user_notification(int this_syscall,
+ struct seccomp_filter *match,
+ const struct seccomp_data *sd)
{
int err;
+ u32 flags = 0;
long ret = 0;
struct seccomp_knotif n = {};
@@ -764,6 +766,7 @@ static void seccomp_do_user_notification(int this_syscall,
if (err == 0) {
ret = n.val;
err = n.error;
+ flags = n.flags;
}
/*
@@ -780,8 +783,14 @@ static void seccomp_do_user_notification(int this_syscall,
list_del(&n.list);
out:
mutex_unlock(&match->notify_lock);
+
+ /* Userspace requests to continue the syscall. */
+ if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
+ return 0;
+
syscall_set_return_value(current, task_pt_regs(current),
err, ret);
+ return -1;
}
static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
@@ -867,8 +876,10 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
return 0;
case SECCOMP_RET_USER_NOTIF:
- seccomp_do_user_notification(this_syscall, match, sd);
- goto skip;
+ if (seccomp_do_user_notification(this_syscall, match, sd))
+ goto skip;
+
+ return 0;
case SECCOMP_RET_LOG:
seccomp_log(this_syscall, 0, action, true);
@@ -1087,7 +1098,11 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
if (copy_from_user(&resp, buf, sizeof(resp)))
return -EFAULT;
- if (resp.flags)
+ if (resp.flags & ~SECCOMP_USER_NOTIF_FLAG_CONTINUE)
+ return -EINVAL;
+
+ if ((resp.flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE) &&
+ (resp.error || resp.val))
return -EINVAL;
ret = mutex_lock_interruptible(&filter->notify_lock);
@@ -1116,6 +1131,7 @@ static long seccomp_notify_send(struct seccomp_filter *filter,
knotif->state = SECCOMP_NOTIFY_REPLIED;
knotif->error = resp.error;
knotif->val = resp.val;
+ knotif->flags = resp.flags;
complete(&knotif->ready);
out:
mutex_unlock(&filter->notify_lock);
--
2.23.0
^ permalink raw reply related
* [PATCH v1 3/3] seccomp: test SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: Christian Brauner @ 2019-09-19 9:59 UTC (permalink / raw)
To: keescook, luto
Cc: jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Christian Brauner,
Tycho Andersen, Tyler Hicks, stable
In-Reply-To: <20190919095903.19370-1-christian.brauner@ubuntu.com>
Test whether a syscall can be performed after having been intercepted by
the seccomp notifier. The test uses dup() and kcmp() since it allows us to
nicely test whether the dup() syscall actually succeeded by comparing whether
the fds refer to the same underlying struct file.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: Tycho Andersen <tycho@tycho.ws>
CC: Tyler Hicks <tyhicks@canonical.com>
Cc: stable@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
---
/* v1 */
- Christian Brauner <christian.brauner@ubuntu.com>:
- adapt to new flag name SECCOMP_USER_NOTIF_FLAG_CONTINUE
/* v0 */
Link: https://lore.kernel.org/r/20190918084833.9369-5-christian.brauner@ubuntu.com
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 102 ++++++++++++++++++
1 file changed, 102 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index e996d7b7fd6e..b0966599acb5 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -44,6 +44,7 @@
#include <sys/times.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
+#include <linux/kcmp.h>
#include <unistd.h>
#include <sys/syscall.h>
@@ -167,6 +168,10 @@ struct seccomp_metadata {
#define SECCOMP_RET_USER_NOTIF 0x7fc00000U
+#ifndef SECCOMP_USER_NOTIF_FLAG_CONTINUE
+#define SECCOMP_USER_NOTIF_FLAG_CONTINUE 0x00000001
+#endif
+
#define SECCOMP_IOC_MAGIC '!'
#define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
#define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
@@ -3481,6 +3486,103 @@ TEST(seccomp_get_notif_sizes)
EXPECT_EQ(sizes.seccomp_notif_resp, sizeof(struct seccomp_notif_resp));
}
+static int filecmp(pid_t pid1, pid_t pid2, int fd1, int fd2)
+{
+#ifdef __NR_kcmp
+ return syscall(__NR_kcmp, pid1, pid2, KCMP_FILE, fd1, fd2);
+#else
+ errno = ENOSYS;
+ return -1;
+#endif
+}
+
+TEST(user_notification_continue)
+{
+ pid_t pid;
+ long ret;
+ int status, listener;
+ struct seccomp_notif req = {};
+ struct seccomp_notif_resp resp = {};
+ struct pollfd pollfd;
+
+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+ ASSERT_EQ(0, ret) {
+ TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+ }
+
+ listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ ASSERT_GE(listener, 0);
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (pid == 0) {
+ int dup_fd, pipe_fds[2];
+ pid_t self;
+
+ ret = pipe(pipe_fds);
+ if (ret < 0)
+ exit(EXIT_FAILURE);
+
+ dup_fd = dup(pipe_fds[0]);
+ if (dup_fd < 0)
+ exit(EXIT_FAILURE);
+
+ self = getpid();
+
+ ret = filecmp(self, self, pipe_fds[0], dup_fd);
+ if (ret)
+ exit(EXIT_FAILURE);
+
+ exit(EXIT_SUCCESS);
+ }
+
+ pollfd.fd = listener;
+ pollfd.events = POLLIN | POLLOUT;
+
+ EXPECT_GT(poll(&pollfd, 1, -1), 0);
+ EXPECT_EQ(pollfd.revents, POLLIN);
+
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
+
+ pollfd.fd = listener;
+ pollfd.events = POLLIN | POLLOUT;
+
+ EXPECT_GT(poll(&pollfd, 1, -1), 0);
+ EXPECT_EQ(pollfd.revents, POLLOUT);
+
+ EXPECT_EQ(req.data.nr, __NR_dup);
+
+ resp.id = req.id;
+ resp.flags = SECCOMP_USER_NOTIF_FLAG_CONTINUE;
+
+ /*
+ * Verify that setting SECCOMP_USER_NOTIF_FLAG_CONTINUE enforces other
+ * args be set to 0.
+ */
+ resp.error = 0;
+ resp.val = USER_NOTIF_MAGIC;
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
+ EXPECT_EQ(errno, EINVAL);
+
+ resp.error = USER_NOTIF_MAGIC;
+ resp.val = 0;
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
+ EXPECT_EQ(errno, EINVAL);
+
+ resp.error = 0;
+ resp.val = 0;
+ EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0) {
+ if (errno == EINVAL)
+ XFAIL(goto skip, "Kernel does not support SECCOMP_USER_NOTIF_FLAG_CONTINUE");
+ }
+
+skip:
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
/*
* TODO:
* - add microbenchmarks
--
2.23.0
^ permalink raw reply related
* [PATCH v1 0/3] seccomp: continue syscall from notifier
From: Christian Brauner @ 2019-09-19 9:59 UTC (permalink / raw)
To: keescook, luto
Cc: jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Christian Brauner
Hey everyone,
This is the patchset coming out of the KSummit session Kees and I gave
in Lisbon last week (cf. [3] which also contains slides with more
details on related things such as deep argument inspection).
The simple idea is to extend the seccomp notifier to allow for the
continuation of a syscall. The rationale for this can be found in the
commit message to [1]. For the curious there is more detail in [2].
This patchset would unblock supervising an extended set of syscalls such
as mount() where a privileged process is supervising the syscalls of a
lesser privileged process and emulates the syscall for the latter in
userspace.
For more comments on security see [1].
Kees, if you prefer a pr the series can be pulled from:
git@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux tags/seccomp-notify-syscall-continue-v5.5
For anyone who wants to play with this it's sitting in:
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=seccomp_syscall_continue
/* v1 */
- Kees Cook <keescook@chromium.org>:
- dropped patch because it is already present in linux-next
[PATCH 2/4] seccomp: add two missing ptrace ifdefines
Link: https://lore.kernel.org/r/20190918084833.9369-3-christian.brauner@ubuntu.com
/* v0 */
Link: https://lore.kernel.org/r/20190918084833.9369-1-christian.brauner@ubuntu.com
Thanks!
Christian
/* References */
[1]: [PATCH 1/3] seccomp: add SECCOMP_USER_NOTIF_FLAG_CONTINUE
[2]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
[3]: https://linuxplumbersconf.org/event/4/contributions/560
Christian Brauner (3):
seccomp: add SECCOMP_USER_NOTIF_FLAG_CONTINUE
seccomp: avoid overflow in implicit constant conversion
seccomp: test SECCOMP_USER_NOTIF_FLAG_CONTINUE
include/uapi/linux/seccomp.h | 20 ++++
kernel/seccomp.c | 28 ++++-
tools/testing/selftests/seccomp/seccomp_bpf.c | 105 +++++++++++++++++-
3 files changed, 146 insertions(+), 7 deletions(-)
--
2.23.0
^ permalink raw reply
* [PATCH net] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jose Abreu @ 2019-09-19 10:09 UTC (permalink / raw)
To: netdev
Cc: Joao Pinto, Jose Abreu, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller, Maxime Coquelin, linux-stm32, linux-arm-kernel,
linux-kernel
The Flow Control selftest is also available with ASYM Pause. Lets add
this check to the test and fix eventual false positive failures.
Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index c56e89e1ae56..4b4b03245f6e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -670,7 +670,7 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
unsigned int pkt_count;
int i, ret = 0;
- if (!phydev || !phydev->pause)
+ if (!phydev || (!phydev->pause && !phydev->asym_pause))
return -EOPNOTSUPP;
tpriv = kzalloc(sizeof(*tpriv), GFP_KERNEL);
--
2.7.4
^ permalink raw reply related
* [PATCH] net-icmp: remove ping_group_range sysctl
From: Maciej Żenczykowski @ 2019-09-19 10:39 UTC (permalink / raw)
To: Maciej Żenczykowski, David S . Miller
Cc: netdev, Kees Cook, Dmitry Vyukov, Eric Dumazet, Mahesh Bandewar,
Lorenzo Colitti
From: Maciej Żenczykowski <maze@google.com>
It is high time to make icmp sockets available to all, and thus allow
utilities like ping, ping6, traceroute and others to not require suid
root nor file system (or otherwise gained) CAP_NET_RAW privs.
While in the past there have been a number of exploits, extensive
syzcaller fuzzing should have made these safe for wide consumption,
and icmp sockets are now safer then having random suid/cap_net_raw
binaries strewn around the file system (and thus you can potentially
mount your root filesystems with nosuid and without file capabilities).
Any remaining issues need to be treated as high priority CVEs anyway,
since for example Fedora 31 is enabling this functionality system wide:
https://fedoraproject.org/wiki/Changes/EnableSysctlPingGroupRange
(core reason: allow ping from rootless user namespace based containers)
There is at least one other highly used Linux distribution which also
unconditionally enables icmp sockets. Thus ping sockets must work and
must be safe.
They can still be disabled via seccomp syscall bpf filters or via the
potentially even more convenient BPF_CGROUP_INET_SOCK_CREATE hook.
I did consider simply changing the in kernel defaults from '1 0' (ie.
disabled) to fully open '0 2**31-1', but the current namespace aware
sysctl is problematic for a number of reasons, and it would take
a huge amount of effort to fix the problems. It's just not worth it.
Another possibility would be to make this sysctl non-namespace aware,
but that seems like a step backward, and besides bpf filters seem
inherently better, and we should be moving forward... icmp sockets
have been around for a long long time now.
In the default configuration, even root/cap_net_raw capable processes
fail to create icmp sockets:
root@athina:~# id
uid=0(root) gid=0(root) groups=0(root),125(pkcs11)
root@athina:~# cat /proc/sys/net/ipv4/ping_group_range
1 0
root@athina:~# strace -ff -esocket ping6 -c 1 ::1
socket(AF_INET6, SOCK_DGRAM, IPPROTO_ICMPV6) = -1 EACCES (Permission denied)
even though they can change the sysctl to allow it:
root@athina:~# echo '0 0' > /proc/sys/net/ipv4/ping_group_range
root@athina:~# cat /proc/sys/net/ipv4/ping_group_range
0 0
root@athina:~# strace -ff -esocket ping6 -c 1 ::1
socket(AF_INET6, SOCK_DGRAM, IPPROTO_ICMPV6) = 3
It only supports non-negative signed 32-bit gids, even though in general
gids are u32 (although, to be fair, the upper half is full of lions).
It is configured via sysctl on a per netns level, as an inclusive gid range,
and is exposed to userspace mapped to the reader/writer's user ns.
This results in all sorts of craziness:
A viewed from root user ns setting of '1 0' (disabled for all) will show
up as '65534 65534' from within a userns where neither 0 nor 1 are mapped
(this is commonly the case: just run 'unshare -Ur' as normal user). If
the viewer is in a group which is not mapped, then 'id' will show that
the user is in gid 65534(nobody), and yet icmp sockets won't work...
A viewed from root user ns setting of '0 2**31-1' (enabled for near all)
will still show up as '65534 65534' from within a userns where neither
0, nor 2**31-1 are mapped (the unpriv user common case for unshare -Ur).
If the viewer is not in any group which is not mapped, then 'id' will
show that the user is not in gid 65534, and yet icmp sockets will actually
work...
As such from within the user/net ns you can't actually tell if it is enabled
or not.
In general, any time '65534' shows up in the sysctl's text format from within
a user ns, it is meaningless: ie. a viewed from root user ns setting of '1 2'
(enabled for gids 1 and 2) will show up as '65534 2' if only gid 2 is mapped...
which makes it *seem* like it's fully disabled... and it'll show up as '1 65534'
if only gid 1 is mapped, which is possibly even more confusing.
Furthermore gids may be mapped into a user ns out of order, so even values
besides the special 'nobody (65534)' are subject to this sort of craziness.
This means that even if you see '1 0' it might not actually be disabled,
since that might map into 0 2**31-1 in the root user ns (note: AFAICT this
requires the sysctl write to happen from outside the user ns).
You would naively think writing 'x y', where 0 <= x < y <= 2**31-1 into the
sysctl would enable ping sockets for gids x..y, but not for gids w & z from
outside that range. But it is fully possible to have w < x < y < z within
the user ns and yet have x < w,z < y within the root user ns, and thus the
sysctl doesn't actually even work, since access is granted to all 4 gids.
This is because all checking happens with compares in the root user ns.
Or x < y within, but x > y outside the user ns, and it'll be fully disabled...
Here's an example of the current state:
maze@m1:~$ id
uid=1000(maze) gid=1000(maze) groups=1000(maze),5000(eng)
maze@m1:~$ unshare -Urn
m1:~# id
uid=0(root) gid=0(root) groups=0(root),65534(nobody)
(we start with the kernel new netns default of 1 0)
m1:~# cat /proc/sys/net/ipv4/ping_group_range
65534 65534 <--- means disabled for all
m1:~# ping6 -c 1 ::1
ping6: icmpv6 open DGRAM socket: Permission denied
(we externally change it to wide open 0 2**31-1)
m1:~# cat /proc/sys/net/ipv4/ping_group_range
65534 65534 <--- means enabled for all
m1:~# ping6 -c 1 ::1
PING ::1(::1) 56 data bytes
64 bytes from ::1: icmp_seq=1 ttl=64 time=0.015 ms
Another example showing you can't trust reading the sysctl:
m1:~# cat /proc/sys/net/ipv4/ping_group_range; id; unshare -Un
0 2147483647
uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),26(tape)
nobody@m1:~$ echo $$; id; cat /proc/sys/net/ipv4/ping_group_range
46444
uid=65534(nobody) gid=65534(nobody) groups=65534(nobody)
65534 65534 <--- really means disabled, since default was '1 0'
(from outside namespace)
m1:~# echo -en "0 0 1\n" | dd bs=4096 of=/proc/46444/uid_map
6 bytes (6 B) copied, 3.6845e-05 s, 163 kB/s
m1:~# echo -en "0 1 1\n$[(1<<31)-1] 0 1\n" | dd bs=4096 of=/proc/46444/gid_map
21 bytes (21 B) copied, 3.5492e-05 s, 592 kB/s
nobody@m1:~$ echo $$; id; cat /proc/sys/net/ipv4/ping_group_range
46444
uid=0(root) gid=2147483647 groups=2147483647,0(root),65534(nobody) <-- 2**31-1 shows up only cause we were in 1(bin)
0 2147483647 <--- still means disabled and not fully enabled like you would expect
One more example which shows the craziness caused by this OR:
if (urange[1] < urange[0] || gid_lt(high, low)) {
low = make_kgid(&init_user_ns, 1);
high = make_kgid(&init_user_ns, 0);
on sysctl write.
m1:~# cat /proc/sys/net/ipv4/ping_group_range; id; unshare -Un
0 2147483647
uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),26(tape)
nobody@m1:~$ echo $$; id; cat /proc/sys/net/ipv4/ping_group_range
49077
uid=65534(nobody) gid=65534(nobody) groups=65534(nobody)
65534 65534 <--- really means '1 0' disabled.
(from outside namespace)
m1:~# echo -en "0 0 1\n" | dd bs=4096 of=/proc/49077/uid_map
6 bytes (6 B) copied, 4.7958e-05 s, 125 kB/s
m1:~# echo -en "0 1 1\n1 0 1\n" | dd bs=4096 of=/proc/49077/gid_map
12 bytes (12 B) copied, 3.358e-05 s, 357 kB/s
nobody@m1:~$ id; cat /proc/sys/net/ipv4/ping_group_range
uid=0(root) gid=1(bin) groups=1(bin),0(root),65534(nobody)
0 1 <--- still really means disabled - we haven't changed the sysctl.
nobody@m1:~$ echo '0 1' > /proc/sys/net/ipv4/ping_group_range
nobody@m1:~$ cat /proc/sys/net/ipv4/ping_group_range
0 1 <--- you'd think this means 0..1, but it still means disabled,
this is because the kernel encoding of '0 1' is '1 0'
which is out of order and triggers the OR.
nobody@m1:~$ echo '1 0' > /proc/sys/net/ipv4/ping_group_range
nobody@m1:~$ cat /proc/sys/net/ipv4/ping_group_range
0 1 <--- huh? and it *still* means disabled because the user
encoding of '1 0' is out of order and triggers the OR...
nobody@m1:~$ echo '0 0' > /proc/sys/net/ipv4/ping_group_range
nobody@m1:~$ cat /proc/sys/net/ipv4/ping_group_range
0 0 <--- correct root user ns gid(1) which is gid(0) here is allowed
nobody@m1:~$ echo '1 1' > /proc/sys/net/ipv4/ping_group_range
nobody@m1:~$ cat /proc/sys/net/ipv4/ping_group_range
1 1 <--- correct root user ns gid(0) which is gid(1) here is allowed
nobody@m1:~$ echo '2 0' > /proc/sys/net/ipv4/ping_group_range
-bash: echo: write error: Invalid argument
nobody@m1:~$ echo '0 2' > /proc/sys/net/ipv4/ping_group_range
-bash: echo: write error: Invalid argument
Additionally if you only have 1 gid 'X' mapped into a user ns, then there
isn't even a way to set the sysctl to disabled (from within), because you
can only write valid gids, and thus you'll always end up with anything
besides 'X X' being invalid, and thus you can only open access up to gid X
as opposed to denying it. (to be fair, it starts disabled, so this is only
a problem if you first enable it)
Cc: Kees Cook <keescook@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Mahesh Bandewar <maheshb@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
include/net/netns/ipv4.h | 7 ----
net/ipv4/af_inet.c | 8 -----
net/ipv4/ping.c | 39 +--------------------
net/ipv4/sysctl_net_ipv4.c | 72 --------------------------------------
4 files changed, 1 insertion(+), 125 deletions(-)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index bc24a8ec1ce5..b65ea5457694 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -24,11 +24,6 @@ struct local_ports {
bool warned;
};
-struct ping_group_range {
- seqlock_t lock;
- kgid_t range[2];
-};
-
struct inet_hashinfo;
struct inet_timewait_death_row {
@@ -190,8 +185,6 @@ struct netns_ipv4 {
int sysctl_igmp_llm_reports;
int sysctl_igmp_qrv;
- struct ping_group_range ping_group_range;
-
atomic_t dev_addr_genid;
#ifdef CONFIG_SYSCTL
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index ed2301ef872e..7e053c2bfef3 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1814,14 +1814,6 @@ static __net_init int inet_init_net(struct net *net)
net->ipv4.ip_local_ports.range[0] = 32768;
net->ipv4.ip_local_ports.range[1] = 60999;
- seqlock_init(&net->ipv4.ping_group_range.lock);
- /*
- * Sane defaults - nobody may create ping sockets.
- * Boot scripts should set this to distro-specific group.
- */
- net->ipv4.ping_group_range.range[0] = make_kgid(&init_user_ns, 1);
- net->ipv4.ping_group_range.range[1] = make_kgid(&init_user_ns, 0);
-
/* Default values for sysctl-controlled parameters.
* We set them here, in case sysctl is not compiled.
*/
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 9d24ef5c5d8f..5e1456161e14 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -234,50 +234,13 @@ static struct sock *ping_lookup(struct net *net, struct sk_buff *skb, u16 ident)
return sk;
}
-static void inet_get_ping_group_range_net(struct net *net, kgid_t *low,
- kgid_t *high)
-{
- kgid_t *data = net->ipv4.ping_group_range.range;
- unsigned int seq;
-
- do {
- seq = read_seqbegin(&net->ipv4.ping_group_range.lock);
-
- *low = data[0];
- *high = data[1];
- } while (read_seqretry(&net->ipv4.ping_group_range.lock, seq));
-}
-
int ping_init_sock(struct sock *sk)
{
- struct net *net = sock_net(sk);
- kgid_t group = current_egid();
- struct group_info *group_info;
- int i;
- kgid_t low, high;
- int ret = 0;
-
if (sk->sk_family == AF_INET6)
sk->sk_ipv6only = 1;
- inet_get_ping_group_range_net(net, &low, &high);
- if (gid_lte(low, group) && gid_lte(group, high))
- return 0;
-
- group_info = get_current_groups();
- for (i = 0; i < group_info->ngroups; i++) {
- kgid_t gid = group_info->gid[i];
-
- if (gid_lte(low, gid) && gid_lte(gid, high))
- goto out_release_group;
- }
-
- ret = -EACCES;
-
-out_release_group:
- put_group_info(group_info);
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(ping_init_sock);
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 0b980e841927..3aa9724e943b 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -45,8 +45,6 @@ static int ip_ttl_min = 1;
static int ip_ttl_max = 255;
static int tcp_syn_retries_min = 1;
static int tcp_syn_retries_max = MAX_TCP_SYNCNT;
-static int ip_ping_group_range_min[] = { 0, 0 };
-static int ip_ping_group_range_max[] = { GID_T_MAX, GID_T_MAX };
static int comp_sack_nr_max = 255;
static u32 u32_max_div_HZ = UINT_MAX / HZ;
static int one_day_secs = 24 * 3600;
@@ -140,69 +138,6 @@ static int ipv4_privileged_ports(struct ctl_table *table, int write,
return ret;
}
-static void inet_get_ping_group_range_table(struct ctl_table *table, kgid_t *low, kgid_t *high)
-{
- kgid_t *data = table->data;
- struct net *net =
- container_of(table->data, struct net, ipv4.ping_group_range.range);
- unsigned int seq;
- do {
- seq = read_seqbegin(&net->ipv4.ping_group_range.lock);
-
- *low = data[0];
- *high = data[1];
- } while (read_seqretry(&net->ipv4.ping_group_range.lock, seq));
-}
-
-/* Update system visible IP port range */
-static void set_ping_group_range(struct ctl_table *table, kgid_t low, kgid_t high)
-{
- kgid_t *data = table->data;
- struct net *net =
- container_of(table->data, struct net, ipv4.ping_group_range.range);
- write_seqlock(&net->ipv4.ping_group_range.lock);
- data[0] = low;
- data[1] = high;
- write_sequnlock(&net->ipv4.ping_group_range.lock);
-}
-
-/* Validate changes from /proc interface. */
-static int ipv4_ping_group_range(struct ctl_table *table, int write,
- void __user *buffer,
- size_t *lenp, loff_t *ppos)
-{
- struct user_namespace *user_ns = current_user_ns();
- int ret;
- gid_t urange[2];
- kgid_t low, high;
- struct ctl_table tmp = {
- .data = &urange,
- .maxlen = sizeof(urange),
- .mode = table->mode,
- .extra1 = &ip_ping_group_range_min,
- .extra2 = &ip_ping_group_range_max,
- };
-
- inet_get_ping_group_range_table(table, &low, &high);
- urange[0] = from_kgid_munged(user_ns, low);
- urange[1] = from_kgid_munged(user_ns, high);
- ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
-
- if (write && ret == 0) {
- low = make_kgid(user_ns, urange[0]);
- high = make_kgid(user_ns, urange[1]);
- if (!gid_valid(low) || !gid_valid(high))
- return -EINVAL;
- if (urange[1] < urange[0] || gid_lt(high, low)) {
- low = make_kgid(&init_user_ns, 1);
- high = make_kgid(&init_user_ns, 0);
- }
- set_ping_group_range(table, low, high);
- }
-
- return ret;
-}
-
static int ipv4_fwd_update_priority(struct ctl_table *table, int write,
void __user *buffer,
size_t *lenp, loff_t *ppos)
@@ -658,13 +593,6 @@ static struct ctl_table ipv4_net_table[] = {
.mode = 0644,
.proc_handler = proc_dointvec
},
- {
- .procname = "ping_group_range",
- .data = &init_net.ipv4.ping_group_range.range,
- .maxlen = sizeof(gid_t)*2,
- .mode = 0644,
- .proc_handler = ipv4_ping_group_range,
- },
#ifdef CONFIG_NET_L3_MASTER_DEV
{
.procname = "raw_l3mdev_accept",
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related
* Re: [PATCH 2/4] seccomp: add two missing ptrace ifdefines
From: Dmitry V. Levin @ 2019-09-19 10:42 UTC (permalink / raw)
To: Kees Cook
Cc: Tyler Hicks, Christian Brauner, luto, jannh, wad, shuah, ast,
daniel, kafai, songliubraving, yhs, linux-kernel, linux-kselftest,
netdev, bpf, Tycho Andersen, stable
In-Reply-To: <201909181031.1EE73B4@keescook>
On Wed, Sep 18, 2019 at 10:33:09AM -0700, Kees Cook wrote:
> On Wed, Sep 18, 2019 at 11:15:12AM +0200, Tyler Hicks wrote:
> > On 2019-09-18 10:48:31, Christian Brauner wrote:
> > > Add tw missing ptrace ifdefines to avoid compilation errors on systems
> > > that do not provide PTRACE_EVENTMSG_SYSCALL_ENTRY or
> > > PTRACE_EVENTMSG_SYSCALL_EXIT or:
> > >
> > > gcc -Wl,-no-as-needed -Wall seccomp_bpf.c -lpthread -o seccomp_bpf
> > > In file included from seccomp_bpf.c:52:0:
> > > seccomp_bpf.c: In function ‘tracer_ptrace’:
> > > seccomp_bpf.c:1792:20: error: ‘PTRACE_EVENTMSG_SYSCALL_ENTRY’ undeclared (first use in this function); did you mean ‘PTRACE_EVENT_CLONE’?
> > > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> > > ^
> > > ../kselftest_harness.h:608:13: note: in definition of macro ‘__EXPECT’
> > > __typeof__(_expected) __exp = (_expected); \
> > > ^~~~~~~~~
> > > seccomp_bpf.c:1792:2: note: in expansion of macro ‘EXPECT_EQ’
> > > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> > > ^~~~~~~~~
> > > seccomp_bpf.c:1792:20: note: each undeclared identifier is reported only once for each function it appears in
> > > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> > > ^
> > > ../kselftest_harness.h:608:13: note: in definition of macro ‘__EXPECT’
> > > __typeof__(_expected) __exp = (_expected); \
> > > ^~~~~~~~~
> > > seccomp_bpf.c:1792:2: note: in expansion of macro ‘EXPECT_EQ’
> > > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> > > ^~~~~~~~~
> > > seccomp_bpf.c:1793:6: error: ‘PTRACE_EVENTMSG_SYSCALL_EXIT’ undeclared (first use in this function); did you mean ‘PTRACE_EVENTMSG_SYSCALL_ENTRY’?
> > > : PTRACE_EVENTMSG_SYSCALL_EXIT, msg);
> > > ^
> > > ../kselftest_harness.h:608:13: note: in definition of macro ‘__EXPECT’
> > > __typeof__(_expected) __exp = (_expected); \
> > > ^~~~~~~~~
> > > seccomp_bpf.c:1792:2: note: in expansion of macro ‘EXPECT_EQ’
> > > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY
> > > ^~~~~~~~~
> > >
> > > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> >
> > I think this Fixes line is incorrect and should be changed to:
> >
> > Fixes: 201766a20e30 ("ptrace: add PTRACE_GET_SYSCALL_INFO request")
> >
> > With that changed,
> >
> > Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
>
> This is actually fixed in -next already (and, yes, with the Fixes line
> Tyler has mentioned):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=69b2d3c5924273a0ae968d3818210fc57a1b9d07
Excuse me, does it mean that you expect each selftest to be self-hosted?
I was (and still is) under impression that selftests should be built
with headers installed from the tree. Is it the case, or is it not?
--
ldv
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox