* [PATCH net-next] geneve: fix ttl inherit type
From: Hangbin Liu @ 2018-09-28 1:09 UTC (permalink / raw)
To: netdev
Cc: David Miller, Stephen Hemminger, David Ahern, Phil Sutter,
Hangbin Liu
Phil pointed out that there is a mismatch between vxlan and geneve ttl
inherit. We should define it as a flag and use nla_put_flag to export this
opiton.
Fixes: 52d0d404d39dd ("geneve: add ttl inherit support")
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
drivers/net/geneve.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 6625fab..09ab2fd 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1100,7 +1100,7 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
[IFLA_GENEVE_UDP_CSUM] = { .type = NLA_U8 },
[IFLA_GENEVE_UDP_ZERO_CSUM6_TX] = { .type = NLA_U8 },
[IFLA_GENEVE_UDP_ZERO_CSUM6_RX] = { .type = NLA_U8 },
- [IFLA_GENEVE_TTL_INHERIT] = { .type = NLA_U8 },
+ [IFLA_GENEVE_TTL_INHERIT] = { .type = NLA_FLAG },
};
static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -1582,7 +1582,7 @@ static size_t geneve_get_size(const struct net_device *dev)
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_CSUM */
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_TX */
nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_UDP_ZERO_CSUM6_RX */
- nla_total_size(sizeof(__u8)) + /* IFLA_GENEVE_TTL_INHERIT */
+ nla_total_size(0) + /* IFLA_GENEVE_TTL_INHERIT */
0;
}
@@ -1636,7 +1636,7 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
#endif
- if (nla_put_u8(skb, IFLA_GENEVE_TTL_INHERIT, ttl_inherit))
+ if (ttl_inherit && nla_put_flag(skb, IFLA_GENEVE_TTL_INHERIT))
goto nla_put_failure;
return 0;
--
2.5.5
^ permalink raw reply related
* Re: [PATCH net] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
From: Samuel Mendoza-Jonas @ 2018-09-28 1:26 UTC (permalink / raw)
To: Justin.Lee1, joel
Cc: amithash, vijaykhemka, linux-aspeed, openbmc, sdasari, netdev,
christian
In-Reply-To: <a2f68c5eb785436ea248cb46b73ea246@AUSX13MPS306.AMER.DELL.COM>
On Thu, 2018-09-27 at 21:08 +0000, Justin.Lee1@Dell.com wrote:
> The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
> to send NC-SI command to the network card.
> Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response.
>
> The work flow is as below.
>
> Request:
> User space application -> Netlink interface (msg)
> -> new Netlink handler - ncsi_send_cmd_nl()
> -> ncsi_xmit_cmd()
> Response:
> Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx()
> -> ncsi_rsp_handler_netlink()
> -> ncsi_send_netlink_rsp ()
> -> Netlink interface (msg)
> -> user space application
> Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout ()
> -> Netlink interface (msg with zero data length)
> -> user space application
> Error:
> Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg)
> -> user space application
>
>
> Signed-off-by: Justin Lee <justin.lee1@dell.com>
>
Hi Justin,
Thanks for posting this on the list! The overall design looks good and so
far looks like it should fit relatively well with the other OEM command
patch. I'll try and run some OEM commands against my machine.
Some comments below:
>
> ---
> include/uapi/linux/ncsi.h | 3 +
> net/ncsi/internal.h | 12 ++-
> net/ncsi/ncsi-aen.c | 10 ++-
> net/ncsi/ncsi-cmd.c | 106 ++++++++++++++++--------
> net/ncsi/ncsi-manage.c | 74 ++++++++++++++---
> net/ncsi/ncsi-netlink.c | 199 +++++++++++++++++++++++++++++++++++++++++++++-
> net/ncsi/ncsi-netlink.h | 4 +
> net/ncsi/ncsi-rsp.c | 70 ++++++++++++++--
> 8 files changed, 420 insertions(+), 58 deletions(-)
>
> diff --git a/include/uapi/linux/ncsi.h b/include/uapi/linux/ncsi.h
> index 4c292ec..4992bfc 100644
> --- a/include/uapi/linux/ncsi.h
> +++ b/include/uapi/linux/ncsi.h
> @@ -30,6 +30,7 @@ enum ncsi_nl_commands {
> NCSI_CMD_PKG_INFO,
> NCSI_CMD_SET_INTERFACE,
> NCSI_CMD_CLEAR_INTERFACE,
> + NCSI_CMD_SEND_CMD,
>
> __NCSI_CMD_AFTER_LAST,
> NCSI_CMD_MAX = __NCSI_CMD_AFTER_LAST - 1
> @@ -43,6 +44,7 @@ enum ncsi_nl_commands {
> * @NCSI_ATTR_PACKAGE_LIST: nested array of NCSI_PKG_ATTR attributes
> * @NCSI_ATTR_PACKAGE_ID: package ID
> * @NCSI_ATTR_CHANNEL_ID: channel ID
> + * @NCSI_ATTR_DATA: command payload
> * @NCSI_ATTR_MAX: highest attribute number
> */
> enum ncsi_nl_attrs {
> @@ -51,6 +53,7 @@ enum ncsi_nl_attrs {
> NCSI_ATTR_PACKAGE_LIST,
> NCSI_ATTR_PACKAGE_ID,
> NCSI_ATTR_CHANNEL_ID,
> + NCSI_ATTR_DATA,
>
> __NCSI_ATTR_AFTER_LAST,
> NCSI_ATTR_MAX = __NCSI_ATTR_AFTER_LAST - 1
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e39..20ce735 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -215,12 +215,17 @@ struct ncsi_request {
> unsigned char id; /* Request ID - 0 to 255 */
> bool used; /* Request that has been assigned */
> unsigned int flags; /* NCSI request property */
> -#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
> +#define NCSI_REQ_FLAG_NETLINK_DRIVEN 2
> struct ncsi_dev_priv *ndp; /* Associated NCSI device */
> struct sk_buff *cmd; /* Associated NCSI command packet */
> struct sk_buff *rsp; /* Associated NCSI response packet */
> struct timer_list timer; /* Timer on waiting for response */
> bool enabled; /* Time has been enabled or not */
> +
> + u32 snd_seq; /* netlink sending sequence number */
> + u32 snd_portid; /* netlink portid of sender */
> + struct nlmsghdr nlhdr; /* netlink message header */
> };
>
> enum {
> @@ -301,10 +306,13 @@ struct ncsi_cmd_arg {
> unsigned short payload; /* Command packet payload length */
> unsigned int req_flags; /* NCSI request properties */
> union {
> - unsigned char bytes[16]; /* Command packet specific data */
> + unsigned char bytes[16]; /* Command packet specific data */
> unsigned short words[8];
> unsigned int dwords[4];
> };
> +
> + unsigned char *data; /* Netlink data */
> + struct genl_info *info; /* Netlink information */
> };
>
> extern struct list_head ncsi_dev_list;
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 25e483e..b5ec193 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -16,6 +16,7 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -73,8 +74,8 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
> ncm->data[2] = data;
> ncm->data[4] = ntohl(lsc->oem_status);
>
> - netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> - nc->id, data & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - pkg %u ch %u state %s\n",
> + nc->package->id, nc->id, data & 0x1 ? "up" : "down");
There's a few places where you've changed or added some debug statements;
these are good but could probably be in a separate patch since not all of
them are related to the OEM command handling.
>
> chained = !list_empty(&nc->link);
> state = nc->state;
> @@ -148,9 +149,10 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> ncm->data[3] = ntohl(hncdsc->status);
> spin_unlock_irqrestore(&nc->lock, flags);
> +
> netdev_dbg(ndp->ndev.dev,
> - "NCSI: host driver %srunning on channel %u\n",
> - ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> + "NCSI: host driver %srunning on pkg %u ch %u\n",
> + ncm->data[3] & 0x1 ? "" : "not ", nc->package->id, nc->id);
>
> return 0;
> }
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63..b291297 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -17,6 +17,7 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -211,42 +212,75 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_pkt *cmd;
> + unsigned char *dest, *source;
> + unsigned short len;
> +
> + /* struct ncsi_cmd_pkt = minimum length
> + * - frame checksum
> + * - Ethernet header
> + * = 64 - 4 - 14 = 46
> + * minimum payload = 46 - ncsi header - ncsi checksum
> + * = 46 - 16 - 4 = 26
> + */
> + len = nca->payload;
> +
> + /* minimum payload length is 26 bytes to meet minimum packet
> + * length 64
> + */
> + if (len < 26)
> + cmd = skb_put_zero(skb, sizeof(*cmd));
> + else
> + cmd = skb_put_zero(skb, len + sizeof(struct ncsi_pkt_hdr) + 4);
> +
> + dest = (unsigned char *)cmd + sizeof(struct ncsi_pkt_hdr);
> + source = (unsigned char *)nca->data + sizeof(struct ncsi_pkt_hdr);
> + memcpy(dest, source, len);
> +
> + ncsi_cmd_build_header(&cmd->cmd.common, nca);
> +
> + return 0;
> +}
This is quite similar to the other patch which is good, they shouldn't
conflict much.
> +
> static struct ncsi_cmd_handler {
> unsigned char type;
> int payload;
> int (*handler)(struct sk_buff *skb,
> struct ncsi_cmd_arg *nca);
> } ncsi_cmd_handlers[] = {
> - { NCSI_PKT_CMD_CIS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SP, 4, ncsi_cmd_handler_sp },
> - { NCSI_PKT_CMD_DP, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_EC, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_DC, 4, ncsi_cmd_handler_dc },
> - { NCSI_PKT_CMD_RC, 4, ncsi_cmd_handler_rc },
> - { NCSI_PKT_CMD_ECNT, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_DCNT, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_AE, 8, ncsi_cmd_handler_ae },
> - { NCSI_PKT_CMD_SL, 8, ncsi_cmd_handler_sl },
> - { NCSI_PKT_CMD_GLS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SVF, 8, ncsi_cmd_handler_svf },
> - { NCSI_PKT_CMD_EV, 4, ncsi_cmd_handler_ev },
> - { NCSI_PKT_CMD_DV, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SMA, 8, ncsi_cmd_handler_sma },
> - { NCSI_PKT_CMD_EBF, 4, ncsi_cmd_handler_ebf },
> - { NCSI_PKT_CMD_DBF, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_EGMF, 4, ncsi_cmd_handler_egmf },
> - { NCSI_PKT_CMD_DGMF, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_SNFC, 4, ncsi_cmd_handler_snfc },
> - { NCSI_PKT_CMD_GVI, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GC, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GP, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GCPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> - { NCSI_PKT_CMD_OEM, 0, NULL },
> - { NCSI_PKT_CMD_PLDM, 0, NULL },
> - { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> + { NCSI_PKT_CMD_CIS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SP, 4, ncsi_cmd_handler_sp },
> + { NCSI_PKT_CMD_DP, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_EC, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_DC, 4, ncsi_cmd_handler_dc },
> + { NCSI_PKT_CMD_RC, 4, ncsi_cmd_handler_rc },
> + { NCSI_PKT_CMD_ECNT, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_DCNT, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_AE, 8, ncsi_cmd_handler_ae },
> + { NCSI_PKT_CMD_SL, 8, ncsi_cmd_handler_sl },
> + { NCSI_PKT_CMD_GLS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SVF, 8, ncsi_cmd_handler_svf },
> + { NCSI_PKT_CMD_EV, 4, ncsi_cmd_handler_ev },
> + { NCSI_PKT_CMD_DV, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SMA, 8, ncsi_cmd_handler_sma },
> + { NCSI_PKT_CMD_EBF, 4, ncsi_cmd_handler_ebf },
> + { NCSI_PKT_CMD_DBF, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_EGMF, 4, ncsi_cmd_handler_egmf },
> + { NCSI_PKT_CMD_DGMF, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_SNFC, 4, ncsi_cmd_handler_snfc },
> + { NCSI_PKT_CMD_GVI, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GC, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GP, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GCPS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GNS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_GPS, 0, ncsi_cmd_handler_default },
> + { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
> + { NCSI_PKT_CMD_PLDM, 0, NULL },
> + { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> };
You've changed the whitespace here which has made the diff unnecessarily
large; please just change the single _OEM line and keep the whitespace
the same.
>
> static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
> @@ -317,11 +351,20 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> }
>
> /* Get packet payload length and allocate the request */
> - nca->payload = nch->payload;
> + if (nch->payload >= 0)
> + nca->payload = nch->payload;
> +
> nr = ncsi_alloc_command(nca);
> if (!nr)
> return -ENOMEM;
>
> + /* track netlink information */
> + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + nr->snd_seq = nca->info->snd_seq;
> + nr->snd_portid = nca->info->snd_portid;
> + nr->nlhdr = *nca->info->nlhdr;
> + }
> +
> /* Prepare the packet */
> nca->id = nr->id;
> ret = nch->handler(nr->cmd, nca);
> @@ -341,6 +384,7 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> * connection a 1 second delay should be sufficient.
> */
> nr->enabled = true;
> +
> mod_timer(&nr->timer, jiffies + 1 * HZ);
>
> /* Send NCSI packet */
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 0912847..6629103 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -19,6 +19,7 @@
> #include <net/addrconf.h>
> #include <net/ipv6.h>
> #include <net/if_inet6.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> @@ -110,8 +111,9 @@ static void ncsi_channel_monitor(struct timer_list *t)
> case NCSI_CHANNEL_MONITOR_WAIT ... NCSI_CHANNEL_MONITOR_WAIT_MAX:
> break;
> default:
> - netdev_err(ndp->ndev.dev, "NCSI Channel %d timed out!\n",
> - nc->id);
> + netdev_err(ndp->ndev.dev, "NCSI: pkg %u ch %u timed out!\n",
> + np->id, nc->id);
> +
> if (!(ndp->flags & NCSI_DEV_HWA)) {
> ncsi_report_link(ndp, true);
> ndp->flags |= NCSI_DEV_RESHUFFLE;
> @@ -143,6 +145,10 @@ void ncsi_start_channel_monitor(struct ncsi_channel *nc)
> {
> unsigned long flags;
>
> + netdev_dbg(nc->package->ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u\n",
> + __func__, nc->package->id, nc->id);
> +
> spin_lock_irqsave(&nc->lock, flags);
> WARN_ON_ONCE(nc->monitor.enabled);
> nc->monitor.enabled = true;
> @@ -156,6 +162,10 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
> {
> unsigned long flags;
>
> + netdev_dbg(nc->package->ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u\n",
> + __func__, nc->package->id, nc->id);
> +
> spin_lock_irqsave(&nc->lock, flags);
> if (!nc->monitor.enabled) {
> spin_unlock_irqrestore(&nc->lock, flags);
> @@ -406,8 +416,13 @@ static void ncsi_request_timeout(struct timer_list *t)
> {
> struct ncsi_request *nr = from_timer(nr, t, timer);
> struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + struct ncsi_cmd_pkt *cmd;
> unsigned long flags;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> /* If the request already had associated response,
> * let the response handler to release it.
> */
> @@ -415,10 +430,23 @@ static void ncsi_request_timeout(struct timer_list *t)
> nr->enabled = false;
> if (nr->rsp || !nr->cmd) {
> spin_unlock_irqrestore(&ndp->lock, flags);
> +
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s - early return\n", __func__);
> +
> return;
> }
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (nr->cmd) {
> + /* Find the package */
> + cmd = (struct ncsi_cmd_pkt *)skb_network_header(nr->cmd);
> + ncsi_find_package_and_channel(ndp, cmd->cmd.common.channel,
> + &np, &nc);
> + ncsi_send_netlink_timeout(nr, np, nc);
> + }
> + }
> +
> /* Release the request */
> ncsi_free_request(nr);
> }
> @@ -432,6 +460,10 @@ static void ncsi_suspend_channel(struct ncsi_dev_priv *ndp)
> unsigned long flags;
> int ret;
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u state %04x\n",
> + __func__, np->id, nc->id, nd->state);
> +
> nca.ndp = ndp;
> nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> @@ -647,6 +679,10 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> unsigned long flags;
> int ret;
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: %s pkg %u ch %u state %04x\n",
> + __func__, np->id, nc->id, nd->state);
> +
> nca.ndp = ndp;
> nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> switch (nd->state) {
> @@ -788,8 +824,9 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> }
> break;
> case ncsi_dev_state_config_done:
> - netdev_dbg(ndp->ndev.dev, "NCSI: channel %u config done\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: pkg %u ch %u config done\n",
> + nc->package->id, nc->id);
> spin_lock_irqsave(&nc->lock, flags);
> if (nc->reconfigure_needed) {
> /* This channel's configuration has been updated
> @@ -815,9 +852,10 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> } else {
> hot_nc = NULL;
> nc->state = NCSI_CHANNEL_INACTIVE;
> +
> netdev_dbg(ndp->ndev.dev,
> - "NCSI: channel %u link down after config\n",
> - nc->id);
> + "NCSI: pkg %u ch %u link down after config\n",
> + nc->package->id, nc->id);
> }
> spin_unlock_irqrestore(&nc->lock, flags);
>
> @@ -853,6 +891,8 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> force_package = ndp->force_package;
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> /* Force a specific channel whether or not it has link if we have been
> * configured to do so
> */
> @@ -861,8 +901,8 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> ncm = &found->modes[NCSI_MODE_LINK];
> if (!(ncm->data[2] & 0x1))
> netdev_info(ndp->ndev.dev,
> - "NCSI: Channel %u forced, but it is link down\n",
> - found->id);
> + "NCSI: pkg %u ch %u forced, but it is link down\n",
> + found->package->id, found->id);
> goto out;
> }
>
> @@ -914,6 +954,7 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> out:
> spin_lock_irqsave(&ndp->lock, flags);
> list_add_tail_rcu(&found->link, &ndp->channel_queue);
> +
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> return ncsi_process_next_channel(ndp);
> @@ -1154,6 +1195,8 @@ static void ncsi_dev_work(struct work_struct *work)
> struct ncsi_dev_priv, work);
> struct ncsi_dev *nd = &ndp->ndev;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> switch (nd->state & ncsi_dev_state_major) {
> case ncsi_dev_state_probe:
> ncsi_probe_channel(ndp);
> @@ -1176,6 +1219,8 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> int old_state;
> unsigned long flags;
>
> + netdev_dbg(ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> spin_lock_irqsave(&ndp->lock, flags);
> nc = list_first_or_null_rcu(&ndp->channel_queue,
> struct ncsi_channel, link);
> @@ -1198,14 +1243,14 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> switch (old_state) {
> case NCSI_CHANNEL_INACTIVE:
> ndp->ndev.state = ncsi_dev_state_config;
> - netdev_dbg(ndp->ndev.dev, "NCSI: configuring channel %u\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: configuring pkg %u ch %u\n",
> + nc->package->id, nc->id);
> ncsi_configure_channel(ndp);
> break;
> case NCSI_CHANNEL_ACTIVE:
> ndp->ndev.state = ncsi_dev_state_suspend;
> - netdev_dbg(ndp->ndev.dev, "NCSI: suspending channel %u\n",
> - nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: suspending pkg %u ch %u\n",
> + nc->package->id, nc->id);
> ncsi_suspend_channel(ndp);
> break;
> default:
> @@ -1225,6 +1270,9 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
> return ncsi_choose_active_channel(ndp);
> }
>
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: No more channels to process\n");
> +
> ncsi_report_link(ndp, false);
> return -ENODEV;
> }
> @@ -1475,6 +1523,7 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
> if (list_empty(&ncsi_dev_list))
> register_inet6addr_notifier(&ncsi_inet6addr_notifier);
> #endif
> +
> list_add_tail_rcu(&ndp->node, &ncsi_dev_list);
> spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>
> @@ -1564,6 +1613,7 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
> if (list_empty(&ncsi_dev_list))
> unregister_inet6addr_notifier(&ncsi_inet6addr_notifier);
> #endif
> +
> spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>
> ncsi_unregister_netlink(nd->dev);
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 45f33d6..ab1a959 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
> @@ -20,6 +20,7 @@
> #include <uapi/linux/ncsi.h>
>
> #include "internal.h"
> +#include "ncsi-pkt.h"
> #include "ncsi-netlink.h"
>
> static struct genl_family ncsi_genl_family;
> @@ -29,6 +30,7 @@ static const struct nla_policy ncsi_genl_policy[NCSI_ATTR_MAX + 1] = {
> [NCSI_ATTR_PACKAGE_LIST] = { .type = NLA_NESTED },
> [NCSI_ATTR_PACKAGE_ID] = { .type = NLA_U32 },
> [NCSI_ATTR_CHANNEL_ID] = { .type = NLA_U32 },
> + [NCSI_ATTR_DATA] = { .type = NLA_BINARY, .len = 2048 },
> };
Is there a particular reason for setting len to 2048, or just a decent
buffer?
>
> static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
> @@ -240,7 +242,7 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
> return 0; /* done */
>
> hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> - &ncsi_genl_family, NLM_F_MULTI, NCSI_CMD_PKG_INFO);
> + &ncsi_genl_family, NLM_F_MULTI, NCSI_CMD_PKG_INFO);
> if (!hdr) {
> rc = -EMSGSIZE;
> goto err;
> @@ -316,8 +318,8 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
> * package
> */
> spin_unlock_irqrestore(&ndp->lock, flags);
> - netdev_info(ndp->ndev.dev, "NCSI: Channel %u does not exist!\n",
> - channel_id);
> + netdev_info(ndp->ndev.dev, "NCSI: pkg %u ch %u does not exist!\n",
> + package_id, channel_id);
> return -ERANGE;
> }
>
> @@ -366,6 +368,191 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
> return 0;
> }
>
> +static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
> +{
> + struct ncsi_dev_priv *ndp;
> +
> + struct ncsi_cmd_arg nca;
> + struct ncsi_pkt_hdr *hdr;
> +
> + u32 package_id, channel_id;
> + unsigned char *data;
> + void *head;
> + int len, ret;
> +
> + if (!info || !info->attrs) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_IFINDEX]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (!info->attrs[NCSI_ATTR_CHANNEL_ID]) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
> + nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
> + if (!ndp) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
> + channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
> +
> + if ((package_id & ~0x07) || (channel_id & ~0x1f)) {
> + ret = -ERANGE;
> + goto out_netlink;
> + }
This is correct but hard to read; we could instead just have
if ((package_id >= 8) ...
which is easier to interpret.
(Probably we should also define the max packages/channels somewhere too)
> +
> + len = nla_len(info->attrs[NCSI_ATTR_DATA]);
> + if (len < sizeof(struct ncsi_pkt_hdr)) {
> + netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
> + package_id);
For statements like these follow the netdev format, eg:
netdev_info(ndp->ndev.dev, "NCSI: no OEM command to send %u\n",
package_id);
> + ret = -EINVAL;
> + goto out_netlink;
> + } else {
> + head = nla_data(info->attrs[NCSI_ATTR_DATA]);
> + data = (unsigned char *)head;
> + }
> +
> + hdr = (struct ncsi_pkt_hdr *)data;
> +
> + nca.ndp = ndp;
> + nca.package = (unsigned char)package_id;
> + nca.channel = (unsigned char)channel_id;
> + nca.type = hdr->type;
> + nca.req_flags = NCSI_REQ_FLAG_NETLINK_DRIVEN;
> + nca.info = info;
> + nca.payload = ntohs(hdr->length);
> + nca.data = data;
> +
> + ret = ncsi_xmit_cmd(&nca);
> +out_netlink:
> + if (ret != 0) {
> + netdev_err(ndp->ndev.dev, "Error %d sending OEM command\n", ret);
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + info->snd_seq, info->snd_portid, info->nlhdr,
> + ret);
> + }
> +out:
> + return ret;
> +}
> +
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> + int rc;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + net = dev_net(nr->rsp->dev);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->rsp->dev->ifindex);
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + rc = nla_put(skb, NCSI_ATTR_DATA, nr->rsp->len, (void *)nr->rsp->data);
> + if (rc)
> + goto err;
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
> +
> +err:
> + kfree_skb(skb);
> + return rc;
> +}
> +
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc)
> +{
> + struct sk_buff *skb;
> + struct net *net;
> + void *hdr;
> +
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: %s\n", __func__);
> +
> + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_put(skb, nr->snd_portid, nr->snd_seq,
> + &ncsi_genl_family, 0, NCSI_CMD_SEND_CMD);
> + if (!hdr) {
> + kfree_skb(skb);
> + return -EMSGSIZE;
> + }
> +
> + net = dev_net(nr->cmd->dev);
> +
> + nla_put_u32(skb, NCSI_ATTR_IFINDEX, nr->cmd->dev->ifindex);
> +
> + if (np)
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID, np->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_PACKAGE_ID,
> + NCSI_PACKAGE_INDEX((((struct ncsi_pkt_hdr *)nr->cmd->data)->channel)));
> +
> + if (nc)
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, nc->id);
> + else
> + nla_put_u32(skb, NCSI_ATTR_CHANNEL_ID, NCSI_RESERVED_CHANNEL);
> +
> + genlmsg_end(skb, hdr);
> + return genlmsg_unicast(net, skb, nr->snd_portid);
How does the receiver of this message know it represents a timeout? Just that
it's missing the NCSI_ATTR_DATA attribute?
> +}
> +
> +int ncsi_send_netlink_err(struct net_device *dev, u32 snd_seq, u32 snd_portid, struct nlmsghdr *nlhdr, int err)
> +{
> + struct sk_buff *skb;
> + struct nlmsghdr *nlh;
> + struct nlmsgerr *nle;
> + struct net *net;
> +
> + skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + net = dev_net(dev);
> +
> + nlh = nlmsg_put(skb, snd_portid, snd_seq,
> + NLMSG_ERROR, sizeof(*nle), 0);
> + nle = (struct nlmsgerr *)nlmsg_data(nlh);
> + nle->error = err;
> + memcpy(&nle->msg, nlhdr, sizeof(*nlh));
> +
> + nlmsg_end(skb, nlh);
> +
> + return nlmsg_unicast(net->genl_sock, skb, snd_portid);
> +}
> +
> static const struct genl_ops ncsi_ops[] = {
> {
> .cmd = NCSI_CMD_PKG_INFO,
> @@ -386,6 +573,12 @@ static const struct genl_ops ncsi_ops[] = {
> .doit = ncsi_clear_interface_nl,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NCSI_CMD_SEND_CMD,
> + .policy = ncsi_genl_policy,
> + .doit = ncsi_send_cmd_nl,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family ncsi_genl_family __ro_after_init = {
> diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
> index 91a5c25..dadaf32 100644
> --- a/net/ncsi/ncsi-netlink.h
> +++ b/net/ncsi/ncsi-netlink.h
> @@ -14,6 +14,10 @@
>
> #include "internal.h"
>
> +int ncsi_send_netlink_rsp(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc);
> +int ncsi_send_netlink_timeout(struct ncsi_request *nr, struct ncsi_package *np, struct ncsi_channel *nc);
> +int ncsi_send_netlink_err(struct net_device *dev, u32 snd_seq, u32 snd_portid, struct nlmsghdr *nlhdr, int err);
> +
> int ncsi_init_netlink(struct net_device *dev);
> int ncsi_unregister_netlink(struct net_device *dev);
>
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 930c1d3..bdf9519 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -16,9 +16,11 @@
> #include <net/ncsi.h>
> #include <net/net_namespace.h>
> #include <net/sock.h>
> +#include <net/genetlink.h>
>
> #include "internal.h"
> #include "ncsi-pkt.h"
> +#include "ncsi-netlink.h"
>
> static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> unsigned short payload)
> @@ -32,15 +34,22 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> * before calling this function.
> */
> h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
> - if (h->common.revision != NCSI_PKT_REVISION)
> +
> + if (h->common.revision != NCSI_PKT_REVISION) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: unsupported header revision\n");
> return -EINVAL;
> - if (ntohs(h->common.length) != payload)
> + }
> + if (ntohs(h->common.length) != payload) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: payload length mismatched\n");
> return -EINVAL;
> + }
>
> /* Check on code and reason */
> if (ntohs(h->code) != NCSI_PKT_RSP_C_COMPLETED ||
> - ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR)
> - return -EINVAL;
> + ntohs(h->reason) != NCSI_PKT_RSP_R_NO_ERROR) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: non zero response/reason code\n");
> + return -EPERM;
> + }
Why the change to EPERM?
>
> /* Validate checksum, which might be zeroes if the
> * sender doesn't support checksum according to NCSI
> @@ -52,8 +61,11 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>
> checksum = ncsi_calculate_checksum((unsigned char *)h,
> sizeof(*h) + payload - 4);
> - if (*pchecksum != htonl(checksum))
> +
> + if (*pchecksum != htonl(checksum)) {
> + netdev_dbg(nr->ndp->ndev.dev, "NCSI: checksum mismatched\n");
> return -EINVAL;
> + }
>
> return 0;
> }
> @@ -900,6 +912,31 @@ static int ncsi_rsp_handler_gpuuid(struct ncsi_request *nr)
> return 0;
> }
>
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> + return 0;
> +}
> +
> +static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
> +{
> + struct ncsi_rsp_pkt *rsp;
> + struct ncsi_dev_priv *ndp = nr->ndp;
> + struct ncsi_package *np;
> + struct ncsi_channel *nc;
> + int ret;
> +
> + /* Find the package */
> + rsp = (struct ncsi_rsp_pkt *)skb_network_header(nr->rsp);
> + ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
> + &np, &nc);
> + if (!np)
> + return -ENODEV;
> +
> + ret = ncsi_send_netlink_rsp(nr, np, nc);
> +
> + return ret;
> +}
> +
> static struct ncsi_rsp_handler {
> unsigned char type;
> int payload;
> @@ -932,7 +969,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
> @@ -950,6 +987,7 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
>
> /* Find the NCSI device */
> nd = ncsi_find_dev(dev);
> +
> ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;
There's a few spots around where you've added or changed whitespace,
please clean these bits up.
> if (!ndp)
> return -ENODEV;
> @@ -1002,6 +1040,15 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_warn(ndp->ndev.dev,
> "NCSI: 'bad' packet ignored for type 0x%x\n",
> hdr->type);
> +
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + if (ret == -EPERM)
> + goto out_netlink;
> + else
> + ncsi_send_netlink_err(ndp->ndev.dev,
> + nr->snd_seq, nr->snd_portid, &nr->nlhdr,
> + ret);
> + }
More netdev formatting, multi-line statements should be like:
ncsi_send_netlink_err(ndp->ndev.dev,
nr->snd_seq, nr->snd_portid, &nr->nlhdr,
ret);
> goto out;
> }
>
> @@ -1011,6 +1058,17 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
> netdev_err(ndp->ndev.dev,
> "NCSI: Handler for packet type 0x%x returned %d\n",
> hdr->type, ret);
> +
> +out_netlink:
> + if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
> + ret = ncsi_rsp_handler_netlink(nr);
> + if (ret) {
> + netdev_err(ndp->ndev.dev,
> + "NCSI: Netlink handler for packet type 0x%x returned %d\n",
> + hdr->type, ret);
> + }
> + }
> +
> out:
> ncsi_free_request(nr);
> return ret;
^ permalink raw reply
* Re: [PATCH net-next v6 00/23] WireGuard: Secure Network Tunnel
From: Ard Biesheuvel @ 2018-09-28 7:52 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Eric Biggers, LKML, Netdev, Linux Crypto Mailing List,
David Miller, Greg Kroah-Hartman
In-Reply-To: <CAHmME9rJ4CewfDEqWh00ZowjWg9TaYpksVSzKSSxoKya-M8_LA@mail.gmail.com>
On 28 September 2018 at 07:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Eric,
>
> On Fri, Sep 28, 2018 at 6:55 AM Eric Biggers <ebiggers@kernel.org> wrote:
>> And you still haven't answered my question about adding a new algorithm that is
>> useful to have in both APIs. You're proposing that in most cases the crypto API
>> part will need to go through Herbert while the implementation will need to go
>> through you/Greg, right? Or will you/Greg be taking both? Or will Herbert take
>> both?
>
> If an implementation enters Zinc, it will go through my tree. If it
> enters the crypto API, it will go through Herbert's tree. If there
> wind up being messy tree dependency and merge timing issues, I can
> work this out in the usual way with Herbert. I'll be sure to discuss
> these edge cases with him when we discuss. I think there's a way to
> handle that with minimum friction.
>
I would also strongly prefer that all crypto work is taken through
Herbert's tree, so we have a coherent view of it before it goes
upstream.
>> A documentation file for Zinc is a good idea. Some of the information in your
>> commit messages should be moved there too.
>
> Will do.
>
>> I'm not trying to "politicize" this, but rather determine your criteria for
>> including algorithms in Zinc, which you haven't made clear. Since for the
>> second time you've avoided answering my question about whether you'd allow the
>> SM4 cipher in Zinc, and you designed WireGuard to be "cryptographically
>> opinionated", and you were one of the loudest voices calling for the Speck
>> cipher to be removed, and your justification for Zinc complains about cipher
>> modes from "90s cryptographers", I think it's reasonable for people to wonder
>> whether as the Zinc (i.e. Linux crypto library) maintainer you will restrict the
>> inclusion of crypto algorithms to the ones you prefer, rather than the ones that
>> users need. Note that the kernel is used by people all over the world and needs
>> to support various standards, protocols, and APIs that use different crypto
>> algorithms, not always the ones we'd like; and different users have different
>> preferences. People need to know whether the Linux kernel's crypto library will
>> meet (or be allowed to meet) their crypto needs.
>
> WireGuard is indeed quite opinionated in its primitive choices, but I
> don't think it'd be wise to apply the same design to Zinc. There are
> APIs where the goal is to have a limited set of high-level functions,
> and have those implemented in only a preferred set of primitives. NaCl
> is a good example of this -- functions like "crypto_secretbox" that
> are actually xsalsapoly under the hood. Zinc doesn't intend to become
> an API like those, but rather to provide the actual primitives for use
> cases where that specific primitive is used. Sometimes the kernel is
> in the business of being able to pick its own crypto -- random.c, tcp
> sequence numbers, big_key.c, etc -- but most of the time the kernel is
> in the business of implementing other people's crypto, for specific
> devices/protocols/diskformats. And for those use cases we need not
> some high-level API like NaCl, but rather direct access to the
> primitives that are required to implement those drivers. WireGuard is
> one such example, but so would be Bluetooth, CIFS/SMB, WiFi, and so
> on. We're in the business of writing drivers, after all. So, no, I
> don't think I'd knock down the addition of a primitive because of a
> simple preference for a different primitive, if it was clearly the
> case that the driver requiring it really benefited from having
> accessible via the plain Zinc function calls. Sorry if I hadn't made
> this clear earlier -- I thought Ard had asked more or less the same
> thing about DES and I answered accordingly, but maybe that wasn't made
> clear enough there.
>
>> > For example, check out the avx blocks function. The radix conversion
>> > happens in a few different places throughout. The reason we need it
>> > separately here is because, unlike userspace, it's possible the kernel
>> > code will transition from 2^26 back to 2^64 as a result of the FPU
>> > context changing.
>>
>> IOW, you had to rewrite the x86 assembly algorithm in C and make it use a
>> different Poly1305 context format. That's a major change, where bugs can be
>> introduced -- and at least one was introduced, in fact. I'd appreciate it if
>> you were more accurate in describing your modifications and the potential risks
>> involved.
>
> A different Poly1305 context format? Not at all - it's using the exact
> same context struct as the assembly. And it's making the same
> conversion that the assembly is. There's not something "different"
> happening; that's the whole point.
>
> Also, this is not some process of frightfully transcribing assembly to
> C and hoping it all works out. This is a careful process of reasoning
> about the limbs, proving that the carries are correct, and that the
> arithmetic done in C actually corresponds to what we want. And then
> finally we check that what we've implemented is indeed the same as
> what the assembly implemented. Finally, as I mentioned, hopefully Andy
> will be folding this back into his implementations sometime soon
> anyway.
>
>> > That's a good idea. I can include some discussion about this as well in
>> > the commit message that introduces the glue code, too, I guess? I've
>> > been hesitant to fill these commit messages up even more, given there
>> > are already so many walls of text and whatnot, but if you think that'd
>> > be useful, I'll do that for v7, and also add comments.
>>
>> Please prefer to put information in the code or documentation rather than in
>> commit messages, when appropriate.
>
> Okay, no problem.
>
>> > This is complete and utter garbage, and I find its insinuations insulting
>> > and ridiculous. There is absolutely no lack of honesty and no double
>> > standard being applied whatsoever. Your attempt to cast doubt about the
>> > quality of standards applied and the integrity of the process is wholly
>> > inappropriate. When I tell you that high standards were applied and that
>> > due-diligence was done in developing a particular patch, I mean what I
>> > say.
>>
>> I
>> disagree that my concerns are "complete and utter garbage". And I think that
>> the fact that you prefer to respond by attacking me, rather than committing to
>> be more accurate in your claims and to treat all contributions fairly, is
>> problematic.
>
> I believe you have the sequence of events wrong. I've stated and made
> that there isn't a double standard, that the standards intend to be
> evenly rigorous, and I solicited your feedback on how I could best
> communicate changes in pre-merged patch series; I did the things
> you've said one should do. But your rhetoric then moved to questioning
> my integrity, and I believe that was uncalled for, inappropriate, and
> not a useful contribution to this mostly productive discussion --
> hence garbage. In other words, I provided an acceptable defense, not
> an attack. But can we move past all this schoolyard nonsense? Cut the
> rhetoric, please; it's really quite overwhelming.
>
>> It's great that you found and fixed this
>> bug on your own, and of course that raises my level of confidence in your work.
>
> Good.
>
>> Still, the v6 patchset still includes your claim that "All implementations have
>> been extensively tested and fuzzed"; so that claim was objectively wrong.
>
> I don't think that claim is wrong. The fuzzing and testing that's been
> done has been extensive, and the fuzzing and testing that continues to
> occur is extensive. As mentioned, the bug was fixed pretty much right
> after git-send-email. If you'd like I can try to space out the patch
> submissions by a little bit longer -- that would probably have various
> benefits -- but given that the netdev code is yet to receive a
> thorough review, I think we've got a bit of time before this is
> merged. The faster-paced patch cycles might inadvertently introduce
> things that get fixed immediately after sending then, unfortunately,
> but I don't think this will be the case with the final series that's
> merged. Though I'm incorporating tons and tons of feedback right now,
> I do look forward to the structure of the series settling down a
> little bit and the pace of suggestions decreasing, so that I can focus
> on auditing and verifying again. But given the dynamism and
> interesting insights of the reviews so far, I think the fast pace has
> actually been useful for elucidating the various expectations and
> suggestions of reviewers. It's most certainly improved this patchset
> in terrific ways.
>
>> Well, it doesn't help that you yourself claim that Zinc stands for
>> "Zx2c4's INsane Cryptolib"...
>
> This expansion of the acronym was intended as a totally ridiculous
> joke. I guess it wasn't received well. I'll remove it from the next
> series. Sorry.
>
As I understood from someone who was at your Kernel Recipes talk, you
mentioned that it actually stands for 'zinc is not crypto/' (note the
slash)
That is needlessly divisive and condescending, so if you want to move
past the rhetoric and the schoolyard nonsense, perhaps you can find a
better name for it? Or just put your stuff into crypto/base/,
crypto/core/ or something like that.
^ permalink raw reply
* RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
From: Y.b. Lu @ 2018-09-28 8:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
netdev@vger.kernel.org, Richard Cochran, David S . Miller,
Ioana Ciocoi Radulescu, Greg Kroah-Hartman
In-Reply-To: <20180927132507.GB23375@lunn.ch>
Hi Andrew,
Thanks a lot for your comments.
Please see my comments inline.
Best regards,
Yangbo Lu
> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, September 27, 2018 9:25 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
>
> On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> > This patch is to move DPAA2 PTP driver out of staging/ since the
> > dpaa2-eth had been moved out.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> > drivers/net/ethernet/freescale/Kconfig | 9 +--------
> > drivers/net/ethernet/freescale/dpaa2/Kconfig | 15
> +++++++++++++++
> > drivers/net/ethernet/freescale/dpaa2/Makefile | 6 ++++--
> > .../ethernet/freescale/dpaa2}/dprtc-cmd.h | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/rtc.c | 0
> > .../rtc => net/ethernet/freescale/dpaa2}/rtc.h | 0
> > drivers/staging/fsl-dpaa2/Kconfig | 8 --------
> > drivers/staging/fsl-dpaa2/Makefile | 1 -
> > drivers/staging/fsl-dpaa2/rtc/Makefile | 7 -------
> > 11 files changed, 20 insertions(+), 26 deletions(-) create mode
> > 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
> > rename drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc.c (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/dprtc.h (100%) rename
> > drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c
> > (100%) rename drivers/{staging/fsl-dpaa2/rtc =>
> > net/ethernet/freescale/dpaa2}/rtc.h (100%)
>
> Hi Yangbo
>
> Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the name,
> change it to ptp.[ch]. Also, some of the function names, and structures,
> rtc_probe->ptp_probe, rtc_remove->ptp_remove, rtc_match_id_table->
> ptp_match_id_table, etc.
[Y.b. Lu] Indeed, it's odd and confusing...
For dpaa2, all hardware resources are allocated and configured through the Management Complex (MC) portals.
MC abstracts most of these resources as DPAA2 objects and exposes ABIs through which they can be configured and controlled.
This ptp timer was named as rtc in MC firmware and APIs as you saw in dprtc.*.
So initially I wrote this driver using rtc as name.
No worries, let me change it in next version.
>
> ptp_dpaa2_adjfreq() probably should return err, not 0.
> ptp_dpaa2_gettime() again does not return the error.
> If fact, it seems like all the main functions ignore errors.
[Y.b. Lu] Will fix the returns in next version.
>
> kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
[Y.b. Lu] Will use devm_kzalloc() in next version.
Can
> ptp_dpaa2_caps be made const?
[Y.b. Lu] Yes. Will change it in next version.
> dpaa2_phc_index does not appear to be used.
[Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of ethtool_ops.
> dev_set_drvdata(dev, NULL); is not needed.
[Y.b. Lu] Will remove it in next version.
> Can rtc_drv be made const?
[Y.b. Lu] Will use const in next version.
> Is rtc.h used by anything other than rtc.c? It seems like it can be removed.
[Y.b. Lu] Let me remove it in next version.
>
> It seems like there is a lot of code in dprtc.c which is unused. rtc.c does nothing
> with interrupts for example. Do you plan to make use of this extra code? Or
> can it be removed leaving just what is needed?
[Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra code is being planed to be used.
>
> struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems very
> odd. And it is not the only example.
[Y.b. Lu] This should depended on MC firmware and APIs I think. Once the MC improves this, the APIs could be updated to fix this.
>
> Andrew
^ permalink raw reply
* Re: [Patch net-next] net_sched: fix an extack message in tcf_block_find()
From: David Ahern @ 2018-09-28 1:51 UTC (permalink / raw)
To: Cong Wang, Eric Dumazet
Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
Vlad Buslov
In-Reply-To: <CAM_iQpX0sxzHT4aiaATPDHRnxWb-x1f_WhV9BQ58GNYZ=DUPHA@mail.gmail.com>
On 9/27/18 3:36 PM, Cong Wang wrote:
> On Thu, Sep 27, 2018 at 2:16 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>>
>> On 09/27/2018 01:42 PM, Cong Wang wrote:
>>> It is clearly a copy-n-paste.
>>>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> ---
>>> net/sched/cls_api.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index 3de47e99b788..8dd7f8af6d54 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -655,7 +655,7 @@ static struct tcf_block *tcf_block_find(struct net *net, struct Qdisc **q,
>>>
>>> *q = qdisc_refcount_inc_nz(*q);
>>> if (!*q) {
>>> - NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
>>> + NL_SET_ERR_MSG(extack, "Can't increase Qdisc refcount");
>>
>>
>> I am not sure it was a copy-n-paste.
>
>
> Make sure you knew there is an exactly same extack message
> (with a same English grammar error).
>
>
>>
>> Qdisc refcount business is kernel internal.
>
> Yeah, but the extack message is already there, this patch doesn't add
> any new extack. Or you are suggesting we should remove it?
IMO the message grammar should be fixed, but the content is correct --
ie, parent qdisc does not exist.
^ permalink raw reply
* Re: KASAN: global-out-of-bounds Read in __aa_lookupn_ns
From: Dmitry Vyukov @ 2018-09-28 8:39 UTC (permalink / raw)
To: syzbot, John Johansen, James Morris, Serge E. Hallyn,
linux-security-module
Cc: David Miller, LKML, netdev, syzkaller-bugs
In-Reply-To: <CACT4Y+YAbon-m=N-UcEVfGj8DxgRAySQz7qLYQWFxQZqv+3JhA@mail.gmail.com>
On Wed, Sep 26, 2018 at 12:06 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Sep 26, 2018 at 9:54 AM, syzbot
> <syzbot+71b6643475f707f93fdc@syzkaller.appspotmail.com> wrote:
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: 02214bfc89c7 Merge tag 'media/v4.19-2' of git://git.kernel..
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=1456f8a1400000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=22a62640793a83c9
>> dashboard link: https://syzkaller.appspot.com/bug?extid=71b6643475f707f93fdc
>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>
> Again misattributed to net. This misattribution should now be fixed by:
> https://github.com/google/syzkaller/commit/db716d6653d073b0abfb51186cd4ac2d5418c9c6
> Adding security/apparmor/policy_ns.c maintainers explicitly.
This is the same as "KASAN: stack-out-of-bounds Read in __aa_lookupn_ns":
https://syzkaller.appspot.com/bug?id=f0d603856d8b3cc9b8e09228f2f548c18ef907ac
right?
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+71b6643475f707f93fdc@syzkaller.appspotmail.com
>>
>> sock_common_setsockopt+0x9a/0xe0 net/core/sock.c:3038
>> __sys_setsockopt+0x1ba/0x3c0 net/socket.c:1902
>> __do_sys_setsockopt net/socket.c:1913 [inline]
>> __se_sys_setsockopt net/socket.c:1910 [inline]
>> __x64_sys_setsockopt+0xbe/0x150 net/socket.c:1910
>> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>> ==================================================================
>> BUG: KASAN: global-out-of-bounds in memcmp+0xe3/0x160 lib/string.c:861
>> Read of size 1 at addr ffffffff88000008 by task syz-executor0/10914
>>
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x457579
>> Code: 1d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
>> 0f 83 eb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007f4c14533c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
>> RAX: ffffffffffffffda RBX: 00007f4c14533c90 RCX: 0000000000457579
>> RDX: 0000000000000040 RSI: 0000000000000000 RDI: 0000000000000003
>> RBP: 000000000072bf00 R08: 0000000000000004 R09: 0000000000000000
>> R10: 0000000020000080 R11: 0000000000000246 R12: 00007f4c145346d4
>> R13: 00000000004c3ed9 R14: 00000000004d6260 R15: 0000000000000004
>> CPU: 0 PID: 10914 Comm: syz-executor0 Not tainted 4.19.0-rc5+ #252
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>> print_address_description.cold.8+0x58/0x1ff mm/kasan/report.c:256
>> kasan_report_error mm/kasan/report.c:354 [inline]
>> kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
>> __asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:430
>> memcmp+0xe3/0x160 lib/string.c:861
>> strnstr+0x4b/0x70 lib/string.c:934
>> __aa_lookupn_ns+0xc1/0x570 security/apparmor/policy_ns.c:209
>> aa_lookupn_ns+0x88/0x1e0 security/apparmor/policy_ns.c:240
>> aa_fqlookupn_profile+0x1b9/0x1010 security/apparmor/policy.c:468
>> fqlookupn_profile+0x80/0xc0 security/apparmor/label.c:1844
>> aa_label_strn_parse+0xa3a/0x1230 security/apparmor/label.c:1908
>> aa_label_parse+0x42/0x50 security/apparmor/label.c:1943
>> aa_change_profile+0x513/0x3510 security/apparmor/domain.c:1362
>> apparmor_setprocattr+0xa8b/0x1150 security/apparmor/lsm.c:656
>> security_setprocattr+0x66/0xc0 security/security.c:1298
>> proc_pid_attr_write+0x301/0x540 fs/proc/base.c:2555
>> __vfs_write+0x119/0x9f0 fs/read_write.c:485
>> vfs_write+0x1fc/0x560 fs/read_write.c:549
>> ksys_write+0x101/0x260 fs/read_write.c:598
>> __do_sys_write fs/read_write.c:610 [inline]
>> __se_sys_write fs/read_write.c:607 [inline]
>> __x64_sys_write+0x73/0xb0 fs/read_write.c:607
>> do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x457579
>> Code: 1d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff
>> 0f 83 eb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007f5a92ec2c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000457579
>> RDX: 000000000000002c RSI: 00000000200000c0 RDI: 0000000000000004
>> RBP: 000000000072bf00 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f5a92ec36d4
>> R13: 00000000004c5454 R14: 00000000004d8c78 R15: 00000000ffffffff
>>
>> CPU: 1 PID: 10921 Comm: syz-executor3 Not tainted 4.19.0-rc5+ #252
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> The buggy address belongs to the variable:
>> __start_rodata+0x8/0x1000
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>>
>> Memory state around the buggy address:
>> ffffffff87ffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ffffffff87ffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> fail_dump lib/fault-inject.c:51 [inline]
>> should_fail.cold.4+0xa/0x17 lib/fault-inject.c:149
>>>
>>> ffffffff88000000: 00 fa fa fa fa fa fa fa 00 01 fa fa fa fa fa fa
>>
>> ^
>> ffffffff88000080: 00 00 00 07 fa fa fa fa 00 04 fa fa fa fa fa fa
>> ffffffff88000100: 05 fa fa fa fa fa fa fa 00 00 00 00 05 fa fa fa
>> ==================================================================
>>
>>
>> ---
>> This bug is generated by a bot. It may contain errors.
>> See https://goo.gl/tpsmEJ for more information about syzbot.
>> syzbot engineers can be reached at syzkaller@googlegroups.com.
>>
>> syzbot will keep track of this bug report. See:
>> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
>> syzbot.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Alexei Starovoitov @ 2018-09-28 8:45 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180926113326.29069-4-guro@fb.com>
On Wed, Sep 26, 2018 at 12:33:19PM +0100, Roman Gushchin wrote:
> This commit introduced per-cpu cgroup local storage.
>
> Per-cpu cgroup local storage is very similar to simple cgroup storage
> (let's call it shared), except all the data is per-cpu.
>
> The main goal of per-cpu variant is to implement super fast
> counters (e.g. packet counters), which don't require neither
> lookups, neither atomic operations.
>
> From userspace's point of view, accessing a per-cpu cgroup storage
> is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> arrays).
>
> Writing to a per-cpu cgroup storage is not atomic, but is performed
> by copying longs, so some minimal atomicity is here, exactly
> as with other per-cpu maps.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf-cgroup.h | 20 ++++-
> include/linux/bpf.h | 1 +
> include/linux/bpf_types.h | 1 +
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/helpers.c | 8 +-
> kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> kernel/bpf/syscall.c | 11 ++-
> kernel/bpf/verifier.c | 15 +++-
> 8 files changed, 177 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 7e0c9a1d48b7..588dd5f0bd85 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> };
>
> struct bpf_cgroup_storage {
> - struct bpf_storage_buffer *buf;
> + union {
> + struct bpf_storage_buffer *buf;
> + void __percpu *percpu_buf;
> + };
> struct bpf_cgroup_storage_map *map;
> struct bpf_cgroup_storage_key key;
> struct list_head list;
> @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> struct bpf_map *map)
> {
> + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> + return BPF_CGROUP_STORAGE_PERCPU;
> +
> return BPF_CGROUP_STORAGE_SHARED;
> }
>
> @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> + void *value, u64 flags);
> +
> /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> ({ \
> @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> static inline void bpf_cgroup_storage_free(
> struct bpf_cgroup_storage *storage) {}
> +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> + void *value) {
> + return 0;
> +}
> +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> + void *key, void *value, u64 flags) {
> + return 0;
> +}
>
> #define cgroup_bpf_enabled (0)
> #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b457fbe7b70b..018299a595c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -274,6 +274,7 @@ struct bpf_prog_offload {
>
> enum bpf_cgroup_storage_type {
> BPF_CGROUP_STORAGE_SHARED,
> + BPF_CGROUP_STORAGE_PERCPU,
> __BPF_CGROUP_STORAGE_MAX
> };
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index c9bd6fb765b0..5432f4c9f50e 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> #endif
> #ifdef CONFIG_CGROUP_BPF
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> #endif
> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..e2070d819e04 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -127,6 +127,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_SOCKHASH,
> BPF_MAP_TYPE_CGROUP_STORAGE,
> BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> };
>
> enum bpf_prog_type {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index e42f8789b7ea..6502115e8f55 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> */
> enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> struct bpf_cgroup_storage *storage;
> + void *ptr;
>
> storage = this_cpu_read(bpf_cgroup_storage[stype]);
>
> - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + ptr = &READ_ONCE(storage->buf)->data[0];
> + else
> + ptr = this_cpu_ptr(storage->percpu_buf);
> +
> + return (unsigned long)ptr;
> }
>
> const struct bpf_func_proto bpf_get_local_storage_proto = {
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 6742292fb39e..c739f6dcc3c2 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> return 0;
> }
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> + void *value)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* per_cpu areas are zero-filled and bpf programs can only
> + * access 'value_size' of them, so copying rounded areas
> + * will not leak any kernel data
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(value + off,
> + per_cpu_ptr(storage->percpu_buf, cpu), size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + if (unlikely(map_flags & BPF_EXIST))
> + return -EINVAL;
that should have been BPF_NOEXIST ?
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* the user space will provide round_up(value_size, 8) bytes that
> + * will be copied into per-cpu area. bpf programs can only access
> + * value_size of it. During lookup the same extra bytes will be
> + * returned or zeros which were zero-filled by percpu_alloc,
> + * so no kernel data leaks possible
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> + value + off, size);
> + off += size;
> + }
> + rcu_read_unlock();
storage_update and storage_copy look essentially the same
with the only difference that src/dst swap.
Would it be possible to combine them ?
Not sure whether #define template would look better.
> + return 0;
> +}
> +
> static int cgroup_storage_get_next_key(struct bpf_map *_map, void *_key,
> void *_next_key)
> {
> @@ -292,55 +357,98 @@ struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(struct bpf_prog *prog,
> {
> struct bpf_cgroup_storage *storage;
> struct bpf_map *map;
> + gfp_t flags;
> + size_t size;
> u32 pages;
>
> map = prog->aux->cgroup_storage[stype];
> if (!map)
> return NULL;
>
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + size = sizeof(struct bpf_storage_buffer) + map->value_size;
> + pages = round_up(sizeof(struct bpf_cgroup_storage) + size,
> + PAGE_SIZE) >> PAGE_SHIFT;
> + } else {
> + size = map->value_size;
> + pages = round_up(round_up(size, 8) * num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> + }
> +
> if (bpf_map_charge_memlock(map, pages))
> return ERR_PTR(-EPERM);
>
> storage = kmalloc_node(sizeof(struct bpf_cgroup_storage),
> __GFP_ZERO | GFP_USER, map->numa_node);
> - if (!storage) {
> - bpf_map_uncharge_memlock(map, pages);
> - return ERR_PTR(-ENOMEM);
> - }
> + if (!storage)
> + goto enomem;
>
> - storage->buf = kmalloc_node(sizeof(struct bpf_storage_buffer) +
> - map->value_size, __GFP_ZERO | GFP_USER,
> - map->numa_node);
> - if (!storage->buf) {
> - bpf_map_uncharge_memlock(map, pages);
> - kfree(storage);
> - return ERR_PTR(-ENOMEM);
> + flags = __GFP_ZERO | GFP_USER;
> +
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + storage->buf = kmalloc_node(size, flags, map->numa_node);
> + if (!storage->buf)
> + goto enomem;
> + } else {
> + storage->percpu_buf = __alloc_percpu_gfp(size, 8, flags);
> + if (!storage->percpu_buf)
> + goto enomem;
> }
>
> storage->map = (struct bpf_cgroup_storage_map *)map;
>
> return storage;
> +
> +enomem:
> + bpf_map_uncharge_memlock(map, pages);
> + kfree(storage);
> + return ERR_PTR(-ENOMEM);
> +}
> +
> +static void free_shared_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + kfree(storage->buf);
> + kfree(storage);
> +}
> +
> +static void free_percpu_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + free_percpu(storage->percpu_buf);
> + kfree(storage);
> }
>
> void bpf_cgroup_storage_free(struct bpf_cgroup_storage *storage)
> {
> - u32 pages;
> + enum bpf_cgroup_storage_type stype;
> struct bpf_map *map;
> + u32 pages;
>
> if (!storage)
> return;
>
> map = &storage->map->map;
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + stype = cgroup_storage_type(map);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + pages = round_up(sizeof(struct bpf_cgroup_storage) +
> + sizeof(struct bpf_storage_buffer) +
> + map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + else
> + pages = round_up(round_up(map->value_size, 8) *
> + num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
storage_alloc and storage_free have subttly different math here.
Please combine them into single helper that computes the number of pages.
In the current form it's hard to tell that the math is actually the same.
^ permalink raw reply
* Re: [PATCH net-next v6 01/23] asm: simd context helper API
From: Ard Biesheuvel @ 2018-09-28 8:49 UTC (permalink / raw)
To: Jason A. Donenfeld, Joe Perches
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
Greg Kroah-Hartman, Samuel Neves, Andy Lutomirski,
Thomas Gleixner, linux-arch
In-Reply-To: <CAKv+Gu9e35+AyktmSq9qeNE0LR83_yrEEB3DiQv0bmyArivqRQ@mail.gmail.com>
(+ Joe)
On 28 September 2018 at 10:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 25 September 2018 at 16:56, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Sometimes it's useful to amortize calls to XSAVE/XRSTOR and the related
>> FPU/SIMD functions over a number of calls, because FPU restoration is
>> quite expensive. This adds a simple header for carrying out this pattern:
>>
>> simd_context_t simd_context;
>>
>> simd_get(&simd_context);
>> while ((item = get_item_from_queue()) != NULL) {
>> encrypt_item(item, simd_context);
>> simd_relax(&simd_context);
>> }
>> simd_put(&simd_context);
>>
>> The relaxation step ensures that we don't trample over preemption, and
>> the get/put API should be a familiar paradigm in the kernel.
>>
>> On the other end, code that actually wants to use SIMD instructions can
>> accept this as a parameter and check it via:
>>
>> void encrypt_item(struct item *item, simd_context_t *simd_context)
>> {
>> if (item->len > LARGE_FOR_SIMD && simd_use(simd_context))
>> wild_simd_code(item);
>> else
>> boring_scalar_code(item);
>> }
>>
>> The actual XSAVE happens during simd_use (and only on the first time),
>> so that if the context is never actually used, no performance penalty is
>> hit.
>>
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>> Cc: Samuel Neves <sneves@dei.uc.pt>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Greg KH <gregkh@linuxfoundation.org>
>> Cc: linux-arch@vger.kernel.org
>> ---
>> arch/alpha/include/asm/Kbuild | 5 ++-
>> arch/arc/include/asm/Kbuild | 1 +
>> arch/arm/include/asm/simd.h | 63 ++++++++++++++++++++++++++++++
>> arch/arm64/include/asm/simd.h | 51 +++++++++++++++++++++---
>> arch/c6x/include/asm/Kbuild | 3 +-
>> arch/h8300/include/asm/Kbuild | 3 +-
>> arch/hexagon/include/asm/Kbuild | 1 +
>> arch/ia64/include/asm/Kbuild | 1 +
>> arch/m68k/include/asm/Kbuild | 1 +
>> arch/microblaze/include/asm/Kbuild | 1 +
>> arch/mips/include/asm/Kbuild | 1 +
>> arch/nds32/include/asm/Kbuild | 7 ++--
>> arch/nios2/include/asm/Kbuild | 1 +
>> arch/openrisc/include/asm/Kbuild | 7 ++--
>> arch/parisc/include/asm/Kbuild | 1 +
>> arch/powerpc/include/asm/Kbuild | 3 +-
>> arch/riscv/include/asm/Kbuild | 3 +-
>> arch/s390/include/asm/Kbuild | 3 +-
>> arch/sh/include/asm/Kbuild | 1 +
>> arch/sparc/include/asm/Kbuild | 1 +
>> arch/um/include/asm/Kbuild | 3 +-
>> arch/unicore32/include/asm/Kbuild | 1 +
>> arch/x86/include/asm/simd.h | 44 ++++++++++++++++++++-
>> arch/xtensa/include/asm/Kbuild | 1 +
>> include/asm-generic/simd.h | 20 ++++++++++
>> include/linux/simd.h | 28 +++++++++++++
>> 26 files changed, 234 insertions(+), 21 deletions(-)
>> create mode 100644 arch/arm/include/asm/simd.h
>> create mode 100644 include/linux/simd.h
>>
>> diff --git a/arch/alpha/include/asm/Kbuild b/arch/alpha/include/asm/Kbuild
>> index 0580cb8c84b2..07b2c1025d34 100644
>> --- a/arch/alpha/include/asm/Kbuild
>> +++ b/arch/alpha/include/asm/Kbuild
>> @@ -2,14 +2,15 @@
>>
>>
>> generic-y += compat.h
>> +generic-y += current.h
>> generic-y += exec.h
>> generic-y += export.h
>> generic-y += fb.h
>> generic-y += irq_work.h
>> +generic-y += kprobes.h
>> generic-y += mcs_spinlock.h
>> generic-y += mm-arch-hooks.h
>> generic-y += preempt.h
>> generic-y += sections.h
>> +generic-y += simd.h
>> generic-y += trace_clock.h
>> -generic-y += current.h
>> -generic-y += kprobes.h
>
> Given that this patch applies to all architectures at once, it is
> probably better to drop the unrelated reordering hunks to avoid
> conflicts.
>
>> diff --git a/arch/arc/include/asm/Kbuild b/arch/arc/include/asm/Kbuild
>> index feed50ce89fa..a7f4255f1649 100644
>> --- a/arch/arc/include/asm/Kbuild
>> +++ b/arch/arc/include/asm/Kbuild
>> @@ -22,6 +22,7 @@ generic-y += parport.h
>> generic-y += pci.h
>> generic-y += percpu.h
>> generic-y += preempt.h
>> +generic-y += simd.h
>> generic-y += topology.h
>> generic-y += trace_clock.h
>> generic-y += user.h
>> diff --git a/arch/arm/include/asm/simd.h b/arch/arm/include/asm/simd.h
>> new file mode 100644
>> index 000000000000..263950dd69cb
>> --- /dev/null
>> +++ b/arch/arm/include/asm/simd.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>> + */
>> +
>> +#include <linux/simd.h>
>> +#ifndef _ASM_SIMD_H
>> +#define _ASM_SIMD_H
>> +
>> +#ifdef CONFIG_KERNEL_MODE_NEON
>> +#include <asm/neon.h>
>> +
>> +static __must_check inline bool may_use_simd(void)
>> +{
>> + return !in_interrupt();
>> +}
>> +
>
> Remember this guy?
>
> https://marc.info/?l=linux-arch&m=149631094625176&w=2
>
> That was never merged, so let's get it right this time.
>
...
>> diff --git a/include/linux/simd.h b/include/linux/simd.h
>> new file mode 100644
>> index 000000000000..33bba21012ff
>> --- /dev/null
>> +++ b/include/linux/simd.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
>> + */
>> +
>> +#ifndef _SIMD_H
>> +#define _SIMD_H
>> +
>> +typedef enum {
>> + HAVE_NO_SIMD = 1 << 0,
>> + HAVE_FULL_SIMD = 1 << 1,
>> + HAVE_SIMD_IN_USE = 1 << 31
>> +} simd_context_t;
>> +
Oh, and another thing (and I'm surprised checkpatch.pl didn't complain
about it): the use of typedef in new code is strongly discouraged.
This policy predates my involvement, so perhaps Joe can elaborate on
the rationale?
>> +#include <linux/sched.h>
>> +#include <asm/simd.h>
>> +
>> +static inline void simd_relax(simd_context_t *ctx)
>> +{
>> +#ifdef CONFIG_PREEMPT
>> + if ((*ctx & HAVE_SIMD_IN_USE) && need_resched()) {
>> + simd_put(ctx);
>> + simd_get(ctx);
>> + }
>> +#endif
>
> Could we return a bool here indicating whether we rescheduled or not?
> In some cases, we could pass that into the asm code as a 'reload'
> param, allowing repeated loads of key schedules, round constant tables
> or S-boxes to be elided.
>
>> +}
>> +
>> +#endif /* _SIMD_H */
>> --
>> 2.19.0
>>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Alexei Starovoitov @ 2018-09-28 8:53 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180926113326.29069-11-guro@fb.com>
On Wed, Sep 26, 2018 at 12:33:26PM +0100, Roman Gushchin wrote:
> This commit adds a bpf kselftest, which demonstrates how percpu
> and shared cgroup local storage can be used for efficient lookup-free
> network accounting.
>
> Cgroup local storage provides generic memory area with a very efficient
> lookup free access. To avoid expensive atomic operations for each
> packet, per-cpu cgroup local storage is used. Each packet is initially
> charged to a per-cpu counter, and only if the counter reaches certain
> value (32 in this case), the charge is moved into the global atomic
> counter. This allows to amortize atomic operations, keeping reasonable
> accuracy.
>
> The test also implements a naive network traffic throttling, mostly to
> demonstrate the possibility of bpf cgroup--based network bandwidth
> control.
>
> Expected output:
> ./test_netcnt
> test_netcnt:PASS
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> tools/testing/selftests/bpf/Makefile | 6 +-
> tools/testing/selftests/bpf/netcnt_common.h | 23 +++
> tools/testing/selftests/bpf/netcnt_prog.c | 71 +++++++++
> tools/testing/selftests/bpf/test_netcnt.c | 153 ++++++++++++++++++++
> 4 files changed, 251 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fd3851d5c079..5443399dd3a1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -23,7 +23,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> - test_socket_cookie test_cgroup_storage test_select_reuseport
> + test_socket_cookie test_cgroup_storage test_select_reuseport \
> + test_netcnt
>
> TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> @@ -35,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> - test_skb_cgroup_id_kern.o bpf_flow.o
> + test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
>
> # Order correspond to 'make run_tests' order
> TEST_PROGS := test_kmod.sh \
> @@ -72,6 +73,7 @@ $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> $(OUTPUT)/test_progs: trace_helpers.c
> $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> +$(OUTPUT)/test_netcnt: cgroup_helpers.c
>
> .PHONY: force
>
> diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> new file mode 100644
> index 000000000000..0e10fc276c2a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/netcnt_common.h
> @@ -0,0 +1,23 @@
> +#ifndef __NETCNT_COMMON_H
> +#define __NETCNT_COMMON_H
> +
> +#include <linux/types.h>
> +
> +#define MAX_PERCPU_PACKETS 32
> +
> +struct percpu_net_cnt {
> + __u64 packets;
> + __u64 bytes;
> +
> + __u64 prev_ts;
> +
> + __u64 prev_packets;
> + __u64 prev_bytes;
> +};
> +
> +struct net_cnt {
> + __u64 packets;
> + __u64 bytes;
> +};
> +
> +#endif
> diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c
> new file mode 100644
> index 000000000000..1198abca1360
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/netcnt_prog.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <linux/version.h>
> +
> +#include "bpf_helpers.h"
> +#include "netcnt_common.h"
> +
> +#define MAX_BPS (3 * 1024 * 1024)
> +
> +#define REFRESH_TIME_NS 100000000
> +#define NS_PER_SEC 1000000000
> +
> +struct bpf_map_def SEC("maps") percpu_netcnt = {
> + .type = BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> + .key_size = sizeof(struct bpf_cgroup_storage_key),
> + .value_size = sizeof(struct percpu_net_cnt),
> +};
> +
> +struct bpf_map_def SEC("maps") netcnt = {
> + .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> + .key_size = sizeof(struct bpf_cgroup_storage_key),
> + .value_size = sizeof(struct net_cnt),
> +};
> +
> +SEC("cgroup/skb")
> +int bpf_nextcnt(struct __sk_buff *skb)
> +{
> + struct percpu_net_cnt *percpu_cnt;
> + char fmt[] = "%d %llu %llu\n";
> + struct net_cnt *cnt;
> + __u64 ts, dt;
> + int ret;
> +
> + cnt = bpf_get_local_storage(&netcnt, 0);
> + percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
> +
> + percpu_cnt->packets++;
> + percpu_cnt->bytes += skb->len;
> +
> + if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
> + __sync_fetch_and_add(&cnt->packets,
> + percpu_cnt->packets);
> + percpu_cnt->packets = 0;
> +
> + __sync_fetch_and_add(&cnt->bytes,
> + percpu_cnt->bytes);
> + percpu_cnt->bytes = 0;
> + }
> +
> + ts = bpf_ktime_get_ns();
> + dt = ts - percpu_cnt->prev_ts;
> +
> + dt *= MAX_BPS;
> + dt /= NS_PER_SEC;
> +
> + if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
> + ret = 1;
> + else
> + ret = 0;
> +
> + if (dt > REFRESH_TIME_NS) {
> + percpu_cnt->prev_ts = ts;
> + percpu_cnt->prev_packets = cnt->packets;
> + percpu_cnt->prev_bytes = cnt->bytes;
> + }
> +
> + return !!ret;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> +__u32 _version SEC("version") = LINUX_VERSION_CODE;
> diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> new file mode 100644
> index 000000000000..aa424f8db466
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_netcnt.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <assert.h>
> +#include <sys/sysinfo.h>
> +#include <sys/time.h>
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +
> +#include "cgroup_helpers.h"
> +#include "bpf_rlimit.h"
> +#include "netcnt_common.h"
> +
> +#define BPF_PROG "./netcnt_prog.o"
> +#define TEST_CGROUP "/test-network-counters/"
> +
> +static int bpf_find_map(const char *test, struct bpf_object *obj,
> + const char *name)
> +{
> + struct bpf_map *map;
> +
> + map = bpf_object__find_map_by_name(obj, name);
> + if (!map) {
> + printf("%s:FAIL:map '%s' not found\n", test, name);
> + return -1;
> + }
> + return bpf_map__fd(map);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + struct percpu_net_cnt *percpu_netcnt;
> + struct bpf_cgroup_storage_key key;
> + int map_fd, percpu_map_fd;
> + int error = EXIT_FAILURE;
> + struct net_cnt netcnt;
> + struct bpf_object *obj;
> + int prog_fd, cgroup_fd;
> + unsigned long packets;
> + int cpu, nproc;
> + __u32 prog_cnt;
> +
> + nproc = get_nprocs_conf();
> + percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> + if (!percpu_netcnt) {
> + printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
> + goto err;
> + }
> +
> + if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
> + &obj, &prog_fd)) {
> + printf("Failed to load bpf program\n");
> + goto out;
> + }
> +
> + if (setup_cgroup_environment()) {
> + printf("Failed to load bpf program\n");
> + goto err;
> + }
> +
> + /* Create a cgroup, get fd, and join it */
> + cgroup_fd = create_and_get_cgroup(TEST_CGROUP);
> + if (!cgroup_fd) {
> + printf("Failed to create test cgroup\n");
> + goto err;
> + }
> +
> + if (join_cgroup(TEST_CGROUP)) {
> + printf("Failed to join cgroup\n");
> + goto err;
> + }
> +
> + /* Attach bpf program */
> + if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
> + printf("Failed to attach bpf program");
> + goto err;
> + }
> +
> + assert(system("ping localhost -s 500 -c 10000 -f -q > /dev/null") == 0);
> +
> + if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
> + &prog_cnt)) {
> + printf("Failed to query attached programs");
> + goto err;
> + }
> +
> + map_fd = bpf_find_map(__func__, obj, "netcnt");
> + if (map_fd < 0) {
> + printf("Failed to find bpf map with net counters");
> + goto err;
> + }
> +
> + percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
> + if (percpu_map_fd < 0) {
> + printf("Failed to find bpf map with percpu net counters");
> + goto err;
> + }
> +
> + if (bpf_map_get_next_key(map_fd, NULL, &key)) {
> + printf("Failed to get key in cgroup storage\n");
> + goto err;
> + }
> +
> + if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> + printf("Failed to lookup cgroup storage\n");
> + goto err;
> + }
> +
> + if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
> + printf("Failed to lookup percpu cgroup storage\n");
> + goto err;
> + }
> +
> + /* Some packets can be still in per-cpu cache, but not more than
> + * MAX_PERCPU_PACKETS.
> + */
> + packets = netcnt.packets;
> + for (cpu = 0; cpu < nproc; cpu++) {
> + if (percpu_netcnt[cpu].packets > 32) {
pls use MAX_PERCPU_PACKETS in the above check.
could you also double check that if that #define is changed to 1k or so
the exact "!= 10000" check below still works as expected?
> + printf("Unexpected percpu value: %llu\n",
> + percpu_netcnt[cpu].packets);
> + goto err;
> + }
> +
> + packets += percpu_netcnt[cpu].packets;
> + }
> +
> + /* No packets should be lost */
> + if (packets != 10000) {
> + printf("Unexpected packet count: %lu\n", packets);
> + goto err;
> + }
> +
> + /* Let's check that bytes counter value is reasonable */
> + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
since packet count is accurate why byte count would vary ?
> + printf("Unexpected bytes count: %llu\n", netcnt.bytes);
> + goto err;
> + }
> +
> + error = 0;
> + printf("test_netcnt:PASS\n");
> +
> +err:
> + cleanup_cgroup_environment();
> + free(percpu_netcnt);
> +
> +out:
> + return error;
> +}
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 1/2] netfilter: nf_tables: add SECMARK support
From: Pablo Neira Ayuso @ 2018-09-28 9:01 UTC (permalink / raw)
To: Christian Göttsche
Cc: kadlec, fw, davem, netfilter-devel, coreteam, netdev,
linux-kernel, paul, sds, eparis, jmorris, serge, selinux,
linux-security-module
In-Reply-To: <20180923182616.11398-1-cgzones@googlemail.com>
On Sun, Sep 23, 2018 at 08:26:15PM +0200, Christian Göttsche wrote:
> Add the ability to set the security context of packets within the nf_tables framework.
> Add a nft_object for holding security contexts in the kernel and manipulating packets on the wire.
>
> Convert the security context strings at rule addition time to security identifiers.
> This is the same behavior like in xt_SECMARK and offers better performance than computing it per packet.
>
> Set the maximum security context length to 256.
Applied, thanks Christian.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Roman Gushchin @ 2018-09-28 10:03 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928084528.i5txkac34pmqvs3p@ast-mbp.dhcp.thefacebook.com>
On Fri, Sep 28, 2018 at 10:45:31AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 26, 2018 at 12:33:19PM +0100, Roman Gushchin wrote:
> > This commit introduced per-cpu cgroup local storage.
> >
> > Per-cpu cgroup local storage is very similar to simple cgroup storage
> > (let's call it shared), except all the data is per-cpu.
> >
> > The main goal of per-cpu variant is to implement super fast
> > counters (e.g. packet counters), which don't require neither
> > lookups, neither atomic operations.
> >
> > From userspace's point of view, accessing a per-cpu cgroup storage
> > is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> > arrays).
> >
> > Writing to a per-cpu cgroup storage is not atomic, but is performed
> > by copying longs, so some minimal atomicity is here, exactly
> > as with other per-cpu maps.
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > include/linux/bpf-cgroup.h | 20 ++++-
> > include/linux/bpf.h | 1 +
> > include/linux/bpf_types.h | 1 +
> > include/uapi/linux/bpf.h | 1 +
> > kernel/bpf/helpers.c | 8 +-
> > kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> > kernel/bpf/syscall.c | 11 ++-
> > kernel/bpf/verifier.c | 15 +++-
> > 8 files changed, 177 insertions(+), 28 deletions(-)
> >
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> > index 7e0c9a1d48b7..588dd5f0bd85 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> > @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> > };
> >
> > struct bpf_cgroup_storage {
> > - struct bpf_storage_buffer *buf;
> > + union {
> > + struct bpf_storage_buffer *buf;
> > + void __percpu *percpu_buf;
> > + };
> > struct bpf_cgroup_storage_map *map;
> > struct bpf_cgroup_storage_key key;
> > struct list_head list;
> > @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> > static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> > struct bpf_map *map)
> > {
> > + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> > + return BPF_CGROUP_STORAGE_PERCPU;
> > +
> > return BPF_CGROUP_STORAGE_SHARED;
> > }
> >
> > @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> > int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> > void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
> >
> > +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> > +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> > + void *value, u64 flags);
> > +
> > /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> > #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> > ({ \
> > @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> > struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> > static inline void bpf_cgroup_storage_free(
> > struct bpf_cgroup_storage *storage) {}
> > +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> > + void *value) {
> > + return 0;
> > +}
> > +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> > + void *key, void *value, u64 flags) {
> > + return 0;
> > +}
> >
> > #define cgroup_bpf_enabled (0)
> > #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index b457fbe7b70b..018299a595c8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -274,6 +274,7 @@ struct bpf_prog_offload {
> >
> > enum bpf_cgroup_storage_type {
> > BPF_CGROUP_STORAGE_SHARED,
> > + BPF_CGROUP_STORAGE_PERCPU,
> > __BPF_CGROUP_STORAGE_MAX
> > };
> >
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index c9bd6fb765b0..5432f4c9f50e 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> > #endif
> > #ifdef CONFIG_CGROUP_BPF
> > BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> > +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> > #endif
> > BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> > BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index aa5ccd2385ed..e2070d819e04 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -127,6 +127,7 @@ enum bpf_map_type {
> > BPF_MAP_TYPE_SOCKHASH,
> > BPF_MAP_TYPE_CGROUP_STORAGE,
> > BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> > + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> > };
> >
> > enum bpf_prog_type {
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index e42f8789b7ea..6502115e8f55 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> > */
> > enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> > struct bpf_cgroup_storage *storage;
> > + void *ptr;
> >
> > storage = this_cpu_read(bpf_cgroup_storage[stype]);
> >
> > - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> > + if (stype == BPF_CGROUP_STORAGE_SHARED)
> > + ptr = &READ_ONCE(storage->buf)->data[0];
> > + else
> > + ptr = this_cpu_ptr(storage->percpu_buf);
> > +
> > + return (unsigned long)ptr;
> > }
> >
> > const struct bpf_func_proto bpf_get_local_storage_proto = {
> > diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> > index 6742292fb39e..c739f6dcc3c2 100644
> > --- a/kernel/bpf/local_storage.c
> > +++ b/kernel/bpf/local_storage.c
> > @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> > return 0;
> > }
> >
> > +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> > + void *value)
> > +{
> > + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> > + struct bpf_cgroup_storage_key *key = _key;
> > + struct bpf_cgroup_storage *storage;
> > + int cpu, off = 0;
> > + u32 size;
> > +
> > + rcu_read_lock();
> > + storage = cgroup_storage_lookup(map, key, false);
> > + if (!storage) {
> > + rcu_read_unlock();
> > + return -ENOENT;
> > + }
> > +
> > + /* per_cpu areas are zero-filled and bpf programs can only
> > + * access 'value_size' of them, so copying rounded areas
> > + * will not leak any kernel data
> > + */
> > + size = round_up(_map->value_size, 8);
> > + for_each_possible_cpu(cpu) {
> > + bpf_long_memcpy(value + off,
> > + per_cpu_ptr(storage->percpu_buf, cpu), size);
> > + off += size;
> > + }
> > + rcu_read_unlock();
> > + return 0;
> > +}
> > +
> > +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> > + void *value, u64 map_flags)
> > +{
> > + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> > + struct bpf_cgroup_storage_key *key = _key;
> > + struct bpf_cgroup_storage *storage;
> > + int cpu, off = 0;
> > + u32 size;
> > +
> > + if (unlikely(map_flags & BPF_EXIST))
> > + return -EINVAL;
>
> that should have been BPF_NOEXIST ?
Yeah, or maybe even better s/&/!= ?
It's probably better to require BPF_EXIST flag to update a cgroup storage?
Agree? If so, let me fix this for both shared and per-cpu versions in
a follow-up patch.
>
> > +
> > + rcu_read_lock();
> > + storage = cgroup_storage_lookup(map, key, false);
> > + if (!storage) {
> > + rcu_read_unlock();
> > + return -ENOENT;
> > + }
> > +
> > + /* the user space will provide round_up(value_size, 8) bytes that
> > + * will be copied into per-cpu area. bpf programs can only access
> > + * value_size of it. During lookup the same extra bytes will be
> > + * returned or zeros which were zero-filled by percpu_alloc,
> > + * so no kernel data leaks possible
> > + */
> > + size = round_up(_map->value_size, 8);
> > + for_each_possible_cpu(cpu) {
> > + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> > + value + off, size);
> > + off += size;
> > + }
> > + rcu_read_unlock();
>
> storage_update and storage_copy look essentially the same
> with the only difference that src/dst swap.
> Would it be possible to combine them ?
> Not sure whether #define template would look better.
I'll try to refactor this and next one in a follow-up patch.
Thanks!
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Roman Gushchin @ 2018-09-28 10:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928085356.56xe7javtd6cdfz6@ast-mbp.dhcp.thefacebook.com>
On Fri, Sep 28, 2018 at 10:53:58AM +0200, Alexei Starovoitov wrote:
> On Wed, Sep 26, 2018 at 12:33:26PM +0100, Roman Gushchin wrote:
> > This commit adds a bpf kselftest, which demonstrates how percpu
> > and shared cgroup local storage can be used for efficient lookup-free
> > network accounting.
> >
> > Cgroup local storage provides generic memory area with a very efficient
> > lookup free access. To avoid expensive atomic operations for each
> > packet, per-cpu cgroup local storage is used. Each packet is initially
> > charged to a per-cpu counter, and only if the counter reaches certain
> > value (32 in this case), the charge is moved into the global atomic
> > counter. This allows to amortize atomic operations, keeping reasonable
> > accuracy.
> >
> > The test also implements a naive network traffic throttling, mostly to
> > demonstrate the possibility of bpf cgroup--based network bandwidth
> > control.
> >
> > Expected output:
> > ./test_netcnt
> > test_netcnt:PASS
> >
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > ---
> > tools/testing/selftests/bpf/Makefile | 6 +-
> > tools/testing/selftests/bpf/netcnt_common.h | 23 +++
> > tools/testing/selftests/bpf/netcnt_prog.c | 71 +++++++++
> > tools/testing/selftests/bpf/test_netcnt.c | 153 ++++++++++++++++++++
> > 4 files changed, 251 insertions(+), 2 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> > create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> > create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index fd3851d5c079..5443399dd3a1 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -23,7 +23,8 @@ $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> > TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> > test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> > test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> > - test_socket_cookie test_cgroup_storage test_select_reuseport
> > + test_socket_cookie test_cgroup_storage test_select_reuseport \
> > + test_netcnt
> >
> > TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> > test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> > @@ -35,7 +36,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
> > test_get_stack_rawtp.o test_sockmap_kern.o test_sockhash_kern.o \
> > test_lwt_seg6local.o sendmsg4_prog.o sendmsg6_prog.o test_lirc_mode2_kern.o \
> > get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
> > - test_skb_cgroup_id_kern.o bpf_flow.o
> > + test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o
> >
> > # Order correspond to 'make run_tests' order
> > TEST_PROGS := test_kmod.sh \
> > @@ -72,6 +73,7 @@ $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> > $(OUTPUT)/test_progs: trace_helpers.c
> > $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> > $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> > +$(OUTPUT)/test_netcnt: cgroup_helpers.c
> >
> > .PHONY: force
> >
> > diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
> > new file mode 100644
> > index 000000000000..0e10fc276c2a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_common.h
> > @@ -0,0 +1,23 @@
> > +#ifndef __NETCNT_COMMON_H
> > +#define __NETCNT_COMMON_H
> > +
> > +#include <linux/types.h>
> > +
> > +#define MAX_PERCPU_PACKETS 32
> > +
> > +struct percpu_net_cnt {
> > + __u64 packets;
> > + __u64 bytes;
> > +
> > + __u64 prev_ts;
> > +
> > + __u64 prev_packets;
> > + __u64 prev_bytes;
> > +};
> > +
> > +struct net_cnt {
> > + __u64 packets;
> > + __u64 bytes;
> > +};
> > +
> > +#endif
> > diff --git a/tools/testing/selftests/bpf/netcnt_prog.c b/tools/testing/selftests/bpf/netcnt_prog.c
> > new file mode 100644
> > index 000000000000..1198abca1360
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/netcnt_prog.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <linux/version.h>
> > +
> > +#include "bpf_helpers.h"
> > +#include "netcnt_common.h"
> > +
> > +#define MAX_BPS (3 * 1024 * 1024)
> > +
> > +#define REFRESH_TIME_NS 100000000
> > +#define NS_PER_SEC 1000000000
> > +
> > +struct bpf_map_def SEC("maps") percpu_netcnt = {
> > + .type = BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> > + .key_size = sizeof(struct bpf_cgroup_storage_key),
> > + .value_size = sizeof(struct percpu_net_cnt),
> > +};
> > +
> > +struct bpf_map_def SEC("maps") netcnt = {
> > + .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> > + .key_size = sizeof(struct bpf_cgroup_storage_key),
> > + .value_size = sizeof(struct net_cnt),
> > +};
> > +
> > +SEC("cgroup/skb")
> > +int bpf_nextcnt(struct __sk_buff *skb)
> > +{
> > + struct percpu_net_cnt *percpu_cnt;
> > + char fmt[] = "%d %llu %llu\n";
> > + struct net_cnt *cnt;
> > + __u64 ts, dt;
> > + int ret;
> > +
> > + cnt = bpf_get_local_storage(&netcnt, 0);
> > + percpu_cnt = bpf_get_local_storage(&percpu_netcnt, 0);
> > +
> > + percpu_cnt->packets++;
> > + percpu_cnt->bytes += skb->len;
> > +
> > + if (percpu_cnt->packets > MAX_PERCPU_PACKETS) {
> > + __sync_fetch_and_add(&cnt->packets,
> > + percpu_cnt->packets);
> > + percpu_cnt->packets = 0;
> > +
> > + __sync_fetch_and_add(&cnt->bytes,
> > + percpu_cnt->bytes);
> > + percpu_cnt->bytes = 0;
> > + }
> > +
> > + ts = bpf_ktime_get_ns();
> > + dt = ts - percpu_cnt->prev_ts;
> > +
> > + dt *= MAX_BPS;
> > + dt /= NS_PER_SEC;
> > +
> > + if (cnt->bytes + percpu_cnt->bytes - percpu_cnt->prev_bytes < dt)
> > + ret = 1;
> > + else
> > + ret = 0;
> > +
> > + if (dt > REFRESH_TIME_NS) {
> > + percpu_cnt->prev_ts = ts;
> > + percpu_cnt->prev_packets = cnt->packets;
> > + percpu_cnt->prev_bytes = cnt->bytes;
> > + }
> > +
> > + return !!ret;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > +__u32 _version SEC("version") = LINUX_VERSION_CODE;
> > diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
> > new file mode 100644
> > index 000000000000..aa424f8db466
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_netcnt.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <assert.h>
> > +#include <sys/sysinfo.h>
> > +#include <sys/time.h>
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "cgroup_helpers.h"
> > +#include "bpf_rlimit.h"
> > +#include "netcnt_common.h"
> > +
> > +#define BPF_PROG "./netcnt_prog.o"
> > +#define TEST_CGROUP "/test-network-counters/"
> > +
> > +static int bpf_find_map(const char *test, struct bpf_object *obj,
> > + const char *name)
> > +{
> > + struct bpf_map *map;
> > +
> > + map = bpf_object__find_map_by_name(obj, name);
> > + if (!map) {
> > + printf("%s:FAIL:map '%s' not found\n", test, name);
> > + return -1;
> > + }
> > + return bpf_map__fd(map);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + struct percpu_net_cnt *percpu_netcnt;
> > + struct bpf_cgroup_storage_key key;
> > + int map_fd, percpu_map_fd;
> > + int error = EXIT_FAILURE;
> > + struct net_cnt netcnt;
> > + struct bpf_object *obj;
> > + int prog_fd, cgroup_fd;
> > + unsigned long packets;
> > + int cpu, nproc;
> > + __u32 prog_cnt;
> > +
> > + nproc = get_nprocs_conf();
> > + percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> > + if (!percpu_netcnt) {
> > + printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
> > + goto err;
> > + }
> > +
> > + if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
> > + &obj, &prog_fd)) {
> > + printf("Failed to load bpf program\n");
> > + goto out;
> > + }
> > +
> > + if (setup_cgroup_environment()) {
> > + printf("Failed to load bpf program\n");
> > + goto err;
> > + }
> > +
> > + /* Create a cgroup, get fd, and join it */
> > + cgroup_fd = create_and_get_cgroup(TEST_CGROUP);
> > + if (!cgroup_fd) {
> > + printf("Failed to create test cgroup\n");
> > + goto err;
> > + }
> > +
> > + if (join_cgroup(TEST_CGROUP)) {
> > + printf("Failed to join cgroup\n");
> > + goto err;
> > + }
> > +
> > + /* Attach bpf program */
> > + if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
> > + printf("Failed to attach bpf program");
> > + goto err;
> > + }
> > +
> > + assert(system("ping localhost -s 500 -c 10000 -f -q > /dev/null") == 0);
> > +
> > + if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
> > + &prog_cnt)) {
> > + printf("Failed to query attached programs");
> > + goto err;
> > + }
> > +
> > + map_fd = bpf_find_map(__func__, obj, "netcnt");
> > + if (map_fd < 0) {
> > + printf("Failed to find bpf map with net counters");
> > + goto err;
> > + }
> > +
> > + percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
> > + if (percpu_map_fd < 0) {
> > + printf("Failed to find bpf map with percpu net counters");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_get_next_key(map_fd, NULL, &key)) {
> > + printf("Failed to get key in cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
> > + printf("Failed to lookup cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
> > + printf("Failed to lookup percpu cgroup storage\n");
> > + goto err;
> > + }
> > +
> > + /* Some packets can be still in per-cpu cache, but not more than
> > + * MAX_PERCPU_PACKETS.
> > + */
> > + packets = netcnt.packets;
> > + for (cpu = 0; cpu < nproc; cpu++) {
> > + if (percpu_netcnt[cpu].packets > 32) {
>
> pls use MAX_PERCPU_PACKETS in the above check.
> could you also double check that if that #define is changed to 1k or so
> the exact "!= 10000" check below still works as expected?
Do you mean adding a new test with a different MAX_PERCPU_PACKETS?
>
> > + printf("Unexpected percpu value: %llu\n",
> > + percpu_netcnt[cpu].packets);
> > + goto err;
>
> > + }
> > +
> > + packets += percpu_netcnt[cpu].packets;
> > + }
> > +
> > + /* No packets should be lost */
> > + if (packets != 10000) {
> > + printf("Unexpected packet count: %lu\n", packets);
> > + goto err;
> > + }
> > +
> > + /* Let's check that bytes counter value is reasonable */
> > + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
>
> since packet count is accurate why byte count would vary ?
Tbh I'm not sure if the size of the packet here can vary depending
on the environment. Is there a nice way to get the expected size?
^ permalink raw reply
* [PATCH net 0/8] rxrpc: Fixes
From: David Howells @ 2018-09-28 10:09 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
Here are some miscellaneous fixes for AF_RXRPC:
(1) Remove a duplicate variable initialisation.
(2) Fix one of the checks made when we decide to set up a new incoming
service call in which a flag is being checked in the wrong field of
the packet header. This check is abstracted out into helper
functions.
(3) Fix RTT gathering. The code has been trying to make use of socket
timestamps, but wasn't actually enabling them. The code has also been
recording a transmit time for the outgoing packet for which we're
going to measure the RTT after sending the message - but we can get
the incoming packet before we get to that and record a negative RTT.
(4) Fix the emission of BUSY packets (we are emitting ABORTs instead).
(5) Improve error checking on incoming packets.
(6) Try to fix a bug in new service call handling whereby a BUG we should
never be able to reach somehow got triggered. Do this by moving much
of the checking as early as possible and not repeating it later
(depends on (5) above).
(7) Fix the sockopts set on a UDP6 socket to include the ones set on a
UDP4 socket so that we receive UDP4 errors and packet-too-large
notifications too.
(8) Fix the distribution of errors so that we do it at the point of
receiving an error in the UDP callback rather than deferring it
thereby cutting short any transmissions that would otherwise occur in
the window.
The patches are tagged here:
git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
rxrpc-fixes-20180928
and can also be found on the following branch:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes
David
---
David Howells (8):
rxrpc: Remove dup code from rxrpc_find_connection_rcu()
rxrpc: Fix checks as to whether we should set up a new call
rxrpc: Fix RTT gathering
rxrpc: Emit BUSY packets when supposed to rather than ABORTs
rxrpc: Improve up-front incoming packet checking
rxrpc: Make service call handling more robust
rxrpc: Fix transport sockopts to get IPv4 errors on an IPv6 socket
rxrpc: Fix error distribution
include/trace/events/rxrpc.h | 4 --
net/rxrpc/ar-internal.h | 36 +++++++++--------
net/rxrpc/call_accept.c | 45 +++++++--------------
net/rxrpc/call_object.c | 2 -
net/rxrpc/conn_client.c | 4 +-
net/rxrpc/conn_object.c | 14 ++++---
net/rxrpc/input.c | 90 ++++++++++++++++++++++++++++++------------
net/rxrpc/local_object.c | 32 ++++++++++-----
net/rxrpc/output.c | 54 +++++++++++++++++--------
net/rxrpc/peer_event.c | 46 +++++----------------
net/rxrpc/peer_object.c | 52 +++++-------------------
net/rxrpc/protocol.h | 15 -------
12 files changed, 189 insertions(+), 205 deletions(-)
^ permalink raw reply
* [PATCH net 1/8] rxrpc: Remove dup code from rxrpc_find_connection_rcu()
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
rxrpc_find_connection_rcu() initialises variable k twice with the same
information. Remove one of the initialisations.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/conn_object.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 77440a356b14..1746b48cb165 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -85,9 +85,6 @@ struct rxrpc_connection *rxrpc_find_connection_rcu(struct rxrpc_local *local,
if (rxrpc_extract_addr_from_skb(local, &srx, skb) < 0)
goto not_found;
- k.epoch = sp->hdr.epoch;
- k.cid = sp->hdr.cid & RXRPC_CIDMASK;
-
/* We may have to handle mixing IPv4 and IPv6 */
if (srx.transport.family != local->srx.transport.family) {
pr_warn_ratelimited("AF_RXRPC: Protocol mismatch %u not %u\n",
^ permalink raw reply related
* [PATCH net 2/8] rxrpc: Fix checks as to whether we should set up a new call
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
There's a check in rxrpc_data_ready() that's checking the CLIENT_INITIATED
flag in the packet type field rather than in the packet flags field.
Fix this by creating a pair of helper functions to check whether the packet
is going to the client or to the server and use them generally.
Fixes: 248f219cb8bc ("rxrpc: Rewrite the data and ack handling code")
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 10 ++++++++++
net/rxrpc/conn_object.c | 2 +-
net/rxrpc/input.c | 12 ++++--------
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index c97558710421..9fcb3e197b14 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -463,6 +463,16 @@ struct rxrpc_connection {
u8 out_clientflag; /* RXRPC_CLIENT_INITIATED if we are client */
};
+static inline bool rxrpc_to_server(const struct rxrpc_skb_priv *sp)
+{
+ return sp->hdr.flags & RXRPC_CLIENT_INITIATED;
+}
+
+static inline bool rxrpc_to_client(const struct rxrpc_skb_priv *sp)
+{
+ return !rxrpc_to_server(sp);
+}
+
/*
* Flags in call->flags.
*/
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 1746b48cb165..390ba50cfab4 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -96,7 +96,7 @@ struct rxrpc_connection *rxrpc_find_connection_rcu(struct rxrpc_local *local,
k.epoch = sp->hdr.epoch;
k.cid = sp->hdr.cid & RXRPC_CIDMASK;
- if (sp->hdr.flags & RXRPC_CLIENT_INITIATED) {
+ if (rxrpc_to_server(sp)) {
/* We need to look up service connections by the full protocol
* parameter set. We look up the peer first as an intermediate
* step and then the connection from the peer's tree.
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index cfdc199c6351..ec299c627f77 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -1177,10 +1177,6 @@ void rxrpc_data_ready(struct sock *udp_sk)
trace_rxrpc_rx_packet(sp);
- _net("Rx RxRPC %s ep=%x call=%x:%x",
- sp->hdr.flags & RXRPC_CLIENT_INITIATED ? "ToServer" : "ToClient",
- sp->hdr.epoch, sp->hdr.cid, sp->hdr.callNumber);
-
if (sp->hdr.type >= RXRPC_N_PACKET_TYPES ||
!((RXRPC_SUPPORTED_PACKET_TYPES >> sp->hdr.type) & 1)) {
_proto("Rx Bad Packet Type %u", sp->hdr.type);
@@ -1189,13 +1185,13 @@ void rxrpc_data_ready(struct sock *udp_sk)
switch (sp->hdr.type) {
case RXRPC_PACKET_TYPE_VERSION:
- if (!(sp->hdr.flags & RXRPC_CLIENT_INITIATED))
+ if (rxrpc_to_client(sp))
goto discard;
rxrpc_post_packet_to_local(local, skb);
goto out;
case RXRPC_PACKET_TYPE_BUSY:
- if (sp->hdr.flags & RXRPC_CLIENT_INITIATED)
+ if (rxrpc_to_server(sp))
goto discard;
/* Fall through */
@@ -1280,7 +1276,7 @@ void rxrpc_data_ready(struct sock *udp_sk)
call = rcu_dereference(chan->call);
if (sp->hdr.callNumber > chan->call_id) {
- if (!(sp->hdr.flags & RXRPC_CLIENT_INITIATED)) {
+ if (rxrpc_to_client(sp)) {
rcu_read_unlock();
goto reject_packet;
}
@@ -1303,7 +1299,7 @@ void rxrpc_data_ready(struct sock *udp_sk)
}
if (!call || atomic_read(&call->usage) == 0) {
- if (!(sp->hdr.type & RXRPC_CLIENT_INITIATED) ||
+ if (rxrpc_to_client(sp) ||
sp->hdr.callNumber == 0 ||
sp->hdr.type != RXRPC_PACKET_TYPE_DATA)
goto bad_message_unlock;
^ permalink raw reply related
* [PATCH net 3/8] rxrpc: Fix RTT gathering
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
Fix RTT information gathering in AF_RXRPC by the following means:
(1) Enable Rx timestamping on the transport socket with SO_TIMESTAMPNS.
(2) If the sk_buff doesn't have a timestamp set when rxrpc_data_ready()
collects it, set it at that point.
(3) Allow ACKs to be requested on the last packet of a client call, but
not a service call. We need to be careful lest we undo:
bf7d620abf22c321208a4da4f435e7af52551a21
Author: David Howells <dhowells@redhat.com>
Date: Thu Oct 6 08:11:51 2016 +0100
rxrpc: Don't request an ACK on the last DATA packet of a call's Tx phase
but that only really applies to service calls that we're handling,
since the client side gets to send the final ACK (or not).
(4) When about to transmit an ACK or DATA packet, record the Tx timestamp
before only; don't update the timestamp afterwards.
(5) Switch the ordering between recording the serial and recording the
timestamp to always set the serial number first. The serial number
shouldn't be seen referenced by an ACK packet until we've transmitted
the packet bearing it - so in the Rx path, we don't need the timestamp
until we've checked the serial number.
Fixes: cf1a6474f807 ("rxrpc: Add per-peer RTT tracker")
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/input.c | 8 ++++++--
net/rxrpc/local_object.c | 9 +++++++++
net/rxrpc/output.c | 31 ++++++++++++++++++-------------
3 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index ec299c627f77..7f9ed3a60b9a 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -622,13 +622,14 @@ static void rxrpc_input_requested_ack(struct rxrpc_call *call,
if (!skb)
continue;
+ sent_at = skb->tstamp;
+ smp_rmb(); /* Read timestamp before serial. */
sp = rxrpc_skb(skb);
if (sp->hdr.serial != orig_serial)
continue;
- smp_rmb();
- sent_at = skb->tstamp;
goto found;
}
+
return;
found:
@@ -1143,6 +1144,9 @@ void rxrpc_data_ready(struct sock *udp_sk)
return;
}
+ if (skb->tstamp == 0)
+ skb->tstamp = ktime_get_real();
+
rxrpc_new_skb(skb, rxrpc_skb_rx_received);
_net("recv skb %p", skb);
diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index 777c3ed4cfc0..81de7d889ffa 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -173,6 +173,15 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
_debug("setsockopt failed");
goto error;
}
+
+ /* We want receive timestamps. */
+ opt = 1;
+ ret = kernel_setsockopt(local->socket, SOL_SOCKET, SO_TIMESTAMPNS,
+ (char *)&opt, sizeof(opt));
+ if (ret < 0) {
+ _debug("setsockopt failed");
+ goto error;
+ }
break;
default:
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index ccf5de160444..8a4da3fe96df 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -124,7 +124,6 @@ int rxrpc_send_ack_packet(struct rxrpc_call *call, bool ping,
struct kvec iov[2];
rxrpc_serial_t serial;
rxrpc_seq_t hard_ack, top;
- ktime_t now;
size_t len, n;
int ret;
u8 reason;
@@ -196,9 +195,7 @@ int rxrpc_send_ack_packet(struct rxrpc_call *call, bool ping,
/* We need to stick a time in before we send the packet in case
* the reply gets back before kernel_sendmsg() completes - but
* asking UDP to send the packet can take a relatively long
- * time, so we update the time after, on the assumption that
- * the packet transmission is more likely to happen towards the
- * end of the kernel_sendmsg() call.
+ * time.
*/
call->ping_time = ktime_get_real();
set_bit(RXRPC_CALL_PINGING, &call->flags);
@@ -206,9 +203,6 @@ int rxrpc_send_ack_packet(struct rxrpc_call *call, bool ping,
}
ret = kernel_sendmsg(conn->params.local->socket, &msg, iov, 2, len);
- now = ktime_get_real();
- if (ping)
- call->ping_time = now;
conn->params.peer->last_tx_at = ktime_get_seconds();
if (ret < 0)
trace_rxrpc_tx_fail(call->debug_id, serial, ret,
@@ -363,8 +357,14 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
/* If our RTT cache needs working on, request an ACK. Also request
* ACKs if a DATA packet appears to have been lost.
+ *
+ * However, we mustn't request an ACK on the last reply packet of a
+ * service call, lest OpenAFS incorrectly send us an ACK with some
+ * soft-ACKs in it and then never follow up with a proper hard ACK.
*/
- if (!(sp->hdr.flags & RXRPC_LAST_PACKET) &&
+ if ((!(sp->hdr.flags & RXRPC_LAST_PACKET) ||
+ rxrpc_to_server(sp)
+ ) &&
(test_and_clear_bit(RXRPC_CALL_EV_ACK_LOST, &call->events) ||
retrans ||
call->cong_mode == RXRPC_CALL_SLOW_START ||
@@ -390,6 +390,11 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
goto send_fragmentable;
down_read(&conn->params.local->defrag_sem);
+
+ sp->hdr.serial = serial;
+ smp_wmb(); /* Set serial before timestamp */
+ skb->tstamp = ktime_get_real();
+
/* send the packet by UDP
* - returns -EMSGSIZE if UDP would have to fragment the packet
* to go out of the interface
@@ -413,12 +418,8 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
trace_rxrpc_tx_data(call, sp->hdr.seq, serial, whdr.flags,
retrans, lost);
if (ret >= 0) {
- ktime_t now = ktime_get_real();
- skb->tstamp = now;
- smp_wmb();
- sp->hdr.serial = serial;
if (whdr.flags & RXRPC_REQUEST_ACK) {
- call->peer->rtt_last_req = now;
+ call->peer->rtt_last_req = skb->tstamp;
trace_rxrpc_rtt_tx(call, rxrpc_rtt_tx_data, serial);
if (call->peer->rtt_usage > 1) {
unsigned long nowj = jiffies, ack_lost_at;
@@ -457,6 +458,10 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
down_write(&conn->params.local->defrag_sem);
+ sp->hdr.serial = serial;
+ smp_wmb(); /* Set serial before timestamp */
+ skb->tstamp = ktime_get_real();
+
switch (conn->params.local->srx.transport.family) {
case AF_INET:
opt = IP_PMTUDISC_DONT;
^ permalink raw reply related
* [PATCH net 5/8] rxrpc: Improve up-front incoming packet checking
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
Do more up-front checking on incoming packets to weed out invalid ones and
also ones aimed at services that we don't support.
Whilst we're at it, replace the clearing of call and skew if we don't find
a connection with just initialising the variables to zero at the top of the
function.
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/input.c | 63 ++++++++++++++++++++++++++++++++++++++++----------
net/rxrpc/protocol.h | 15 ------------
2 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index b0f12471f5e7..a569e9e010d1 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -1125,12 +1125,13 @@ void rxrpc_data_ready(struct sock *udp_sk)
{
struct rxrpc_connection *conn;
struct rxrpc_channel *chan;
- struct rxrpc_call *call;
+ struct rxrpc_call *call = NULL;
struct rxrpc_skb_priv *sp;
struct rxrpc_local *local = udp_sk->sk_user_data;
+ struct rxrpc_sock *rx;
struct sk_buff *skb;
unsigned int channel;
- int ret, skew;
+ int ret, skew = 0;
_enter("%p", udp_sk);
@@ -1181,12 +1182,6 @@ void rxrpc_data_ready(struct sock *udp_sk)
trace_rxrpc_rx_packet(sp);
- if (sp->hdr.type >= RXRPC_N_PACKET_TYPES ||
- !((RXRPC_SUPPORTED_PACKET_TYPES >> sp->hdr.type) & 1)) {
- _proto("Rx Bad Packet Type %u", sp->hdr.type);
- goto bad_message;
- }
-
switch (sp->hdr.type) {
case RXRPC_PACKET_TYPE_VERSION:
if (rxrpc_to_client(sp))
@@ -1198,24 +1193,63 @@ void rxrpc_data_ready(struct sock *udp_sk)
if (rxrpc_to_server(sp))
goto discard;
/* Fall through */
+ case RXRPC_PACKET_TYPE_ACK:
+ case RXRPC_PACKET_TYPE_ACKALL:
+ if (sp->hdr.callNumber == 0)
+ goto bad_message;
+ /* Fall through */
+ case RXRPC_PACKET_TYPE_ABORT:
+ break;
case RXRPC_PACKET_TYPE_DATA:
- if (sp->hdr.callNumber == 0)
+ if (sp->hdr.callNumber == 0 ||
+ sp->hdr.seq == 0)
goto bad_message;
if (sp->hdr.flags & RXRPC_JUMBO_PACKET &&
!rxrpc_validate_jumbo(skb))
goto bad_message;
break;
+ case RXRPC_PACKET_TYPE_CHALLENGE:
+ if (rxrpc_to_server(sp))
+ goto discard;
+ break;
+ case RXRPC_PACKET_TYPE_RESPONSE:
+ if (rxrpc_to_client(sp))
+ goto discard;
+ break;
+
/* Packet types 9-11 should just be ignored. */
case RXRPC_PACKET_TYPE_PARAMS:
case RXRPC_PACKET_TYPE_10:
case RXRPC_PACKET_TYPE_11:
goto discard;
+
+ default:
+ _proto("Rx Bad Packet Type %u", sp->hdr.type);
+ goto bad_message;
}
+ if (sp->hdr.serviceId == 0)
+ goto bad_message;
+
rcu_read_lock();
+ if (rxrpc_to_server(sp)) {
+ /* Weed out packets to services we're not offering. Packets
+ * that would begin a call are explicitly rejected and the rest
+ * are just discarded.
+ */
+ rx = rcu_dereference(local->service);
+ if (!rx || (sp->hdr.serviceId != rx->srx.srx_service &&
+ sp->hdr.serviceId != rx->second_service)) {
+ if (sp->hdr.type == RXRPC_PACKET_TYPE_DATA &&
+ sp->hdr.seq == 1)
+ goto unsupported_service;
+ goto discard_unlock;
+ }
+ }
+
conn = rxrpc_find_connection_rcu(local, skb);
if (conn) {
if (sp->hdr.securityIndex != conn->security_ix)
@@ -1297,14 +1331,10 @@ void rxrpc_data_ready(struct sock *udp_sk)
if (!test_bit(RXRPC_CALL_RX_HEARD, &call->flags))
set_bit(RXRPC_CALL_RX_HEARD, &call->flags);
}
- } else {
- skew = 0;
- call = NULL;
}
if (!call || atomic_read(&call->usage) == 0) {
if (rxrpc_to_client(sp) ||
- sp->hdr.callNumber == 0 ||
sp->hdr.type != RXRPC_PACKET_TYPE_DATA)
goto bad_message_unlock;
if (sp->hdr.seq != 1)
@@ -1340,6 +1370,13 @@ void rxrpc_data_ready(struct sock *udp_sk)
skb->priority = RXKADINCONSISTENCY;
goto post_abort;
+unsupported_service:
+ rcu_read_unlock();
+ trace_rxrpc_abort(0, "INV", sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
+ RX_INVALID_OPERATION, EOPNOTSUPP);
+ skb->priority = RX_INVALID_OPERATION;
+ goto post_abort;
+
reupgrade:
rcu_read_unlock();
trace_rxrpc_abort(0, "UPG", sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
diff --git a/net/rxrpc/protocol.h b/net/rxrpc/protocol.h
index 93da73bf7098..f9cb83c938f3 100644
--- a/net/rxrpc/protocol.h
+++ b/net/rxrpc/protocol.h
@@ -50,7 +50,6 @@ struct rxrpc_wire_header {
#define RXRPC_PACKET_TYPE_10 10 /* Ignored */
#define RXRPC_PACKET_TYPE_11 11 /* Ignored */
#define RXRPC_PACKET_TYPE_VERSION 13 /* version string request */
-#define RXRPC_N_PACKET_TYPES 14 /* number of packet types (incl type 0) */
uint8_t flags; /* packet flags */
#define RXRPC_CLIENT_INITIATED 0x01 /* signifies a packet generated by a client */
@@ -72,20 +71,6 @@ struct rxrpc_wire_header {
} __packed;
-#define RXRPC_SUPPORTED_PACKET_TYPES ( \
- (1 << RXRPC_PACKET_TYPE_DATA) | \
- (1 << RXRPC_PACKET_TYPE_ACK) | \
- (1 << RXRPC_PACKET_TYPE_BUSY) | \
- (1 << RXRPC_PACKET_TYPE_ABORT) | \
- (1 << RXRPC_PACKET_TYPE_ACKALL) | \
- (1 << RXRPC_PACKET_TYPE_CHALLENGE) | \
- (1 << RXRPC_PACKET_TYPE_RESPONSE) | \
- /*(1 << RXRPC_PACKET_TYPE_DEBUG) | */ \
- (1 << RXRPC_PACKET_TYPE_PARAMS) | \
- (1 << RXRPC_PACKET_TYPE_10) | \
- (1 << RXRPC_PACKET_TYPE_11) | \
- (1 << RXRPC_PACKET_TYPE_VERSION))
-
/*****************************************************************************/
/*
* jumbo packet secondary header
^ permalink raw reply related
* [PATCH net 6/8] rxrpc: Make service call handling more robust
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
Make the following changes to improve the robustness of the code that sets
up a new service call:
(1) Cache the rxrpc_sock struct obtained in rxrpc_data_ready() to do a
service ID check and pass that along to rxrpc_new_incoming_call().
This means that I can remove the check from rxrpc_new_incoming_call()
without the need to worry about the socket attached to the local
endpoint getting replaced - which would invalidate the check.
(2) Cache the rxrpc_peer struct, thereby allowing the peer search to be
done once. The peer is passed to rxrpc_new_incoming_call(), thereby
saving the need to repeat the search.
This also reduces the possibility of rxrpc_publish_service_conn()
BUG()'ing due to the detection of a duplicate connection, despite the
initial search done by rxrpc_find_connection_rcu() having turned up
nothing.
This BUG() shouldn't ever get hit since rxrpc_data_ready() *should* be
non-reentrant and the result of the initial search should still hold
true, but it has proven possible to hit.
I *think* this may be due to __rxrpc_lookup_peer_rcu() cutting short
the iteration over the hash table if it finds a matching peer with a
zero usage count, but I don't know for sure since it's only ever been
hit once that I know of.
Another possibility is that a bug in rxrpc_data_ready() that checked
the wrong byte in the header for the RXRPC_CLIENT_INITIATED flag
might've let through a packet that caused a spurious and invalid call
to be set up. That is addressed in another patch.
(3) Fix __rxrpc_lookup_peer_rcu() to skip peer records that have a zero
usage count rather than stopping and returning not found, just in case
there's another peer record behind it in the bucket.
(4) Don't search the peer records in rxrpc_alloc_incoming_call(), but
rather either use the peer cached in (2) or, if one wasn't found,
preemptively install a new one.
Fixes: 8496af50eb38 ("rxrpc: Use RCU to access a peer's service connection tree")
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/ar-internal.h | 8 +++++---
net/rxrpc/call_accept.c | 41 ++++++++++++-----------------------------
net/rxrpc/conn_object.c | 7 ++++++-
net/rxrpc/input.c | 7 ++++---
net/rxrpc/peer_object.c | 35 +++++++++++------------------------
5 files changed, 38 insertions(+), 60 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index e8861cb78070..c72686193d83 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -722,6 +722,8 @@ extern struct workqueue_struct *rxrpc_workqueue;
int rxrpc_service_prealloc(struct rxrpc_sock *, gfp_t);
void rxrpc_discard_prealloc(struct rxrpc_sock *);
struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *,
+ struct rxrpc_sock *,
+ struct rxrpc_peer *,
struct rxrpc_connection *,
struct sk_buff *);
void rxrpc_accept_incoming_calls(struct rxrpc_local *);
@@ -913,7 +915,8 @@ extern unsigned int rxrpc_closed_conn_expiry;
struct rxrpc_connection *rxrpc_alloc_connection(gfp_t);
struct rxrpc_connection *rxrpc_find_connection_rcu(struct rxrpc_local *,
- struct sk_buff *);
+ struct sk_buff *,
+ struct rxrpc_peer **);
void __rxrpc_disconnect_call(struct rxrpc_connection *, struct rxrpc_call *);
void rxrpc_disconnect_call(struct rxrpc_call *);
void rxrpc_kill_connection(struct rxrpc_connection *);
@@ -1049,8 +1052,7 @@ struct rxrpc_peer *rxrpc_lookup_peer_rcu(struct rxrpc_local *,
struct rxrpc_peer *rxrpc_lookup_peer(struct rxrpc_local *,
struct sockaddr_rxrpc *, gfp_t);
struct rxrpc_peer *rxrpc_alloc_peer(struct rxrpc_local *, gfp_t);
-struct rxrpc_peer *rxrpc_lookup_incoming_peer(struct rxrpc_local *,
- struct rxrpc_peer *);
+void rxrpc_new_incoming_peer(struct rxrpc_local *, struct rxrpc_peer *);
void rxrpc_destroy_all_peers(struct rxrpc_net *);
struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *);
struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *);
diff --git a/net/rxrpc/call_accept.c b/net/rxrpc/call_accept.c
index e88f131c1d7f..9c7f26d06a52 100644
--- a/net/rxrpc/call_accept.c
+++ b/net/rxrpc/call_accept.c
@@ -249,11 +249,11 @@ void rxrpc_discard_prealloc(struct rxrpc_sock *rx)
*/
static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
struct rxrpc_local *local,
+ struct rxrpc_peer *peer,
struct rxrpc_connection *conn,
struct sk_buff *skb)
{
struct rxrpc_backlog *b = rx->backlog;
- struct rxrpc_peer *peer, *xpeer;
struct rxrpc_call *call;
unsigned short call_head, conn_head, peer_head;
unsigned short call_tail, conn_tail, peer_tail;
@@ -276,21 +276,18 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
return NULL;
if (!conn) {
- /* No connection. We're going to need a peer to start off
- * with. If one doesn't yet exist, use a spare from the
- * preallocation set. We dump the address into the spare in
- * anticipation - and to save on stack space.
- */
- xpeer = b->peer_backlog[peer_tail];
- if (rxrpc_extract_addr_from_skb(local, &xpeer->srx, skb) < 0)
- return NULL;
-
- peer = rxrpc_lookup_incoming_peer(local, xpeer);
- if (peer == xpeer) {
+ if (peer && !rxrpc_get_peer_maybe(peer))
+ peer = NULL;
+ if (!peer) {
+ peer = b->peer_backlog[peer_tail];
+ if (rxrpc_extract_addr_from_skb(local, &peer->srx, skb) < 0)
+ return NULL;
b->peer_backlog[peer_tail] = NULL;
smp_store_release(&b->peer_backlog_tail,
(peer_tail + 1) &
(RXRPC_BACKLOG_MAX - 1));
+
+ rxrpc_new_incoming_peer(local, peer);
}
/* Now allocate and set up the connection */
@@ -335,30 +332,16 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
* The call is returned with the user access mutex held.
*/
struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
+ struct rxrpc_sock *rx,
+ struct rxrpc_peer *peer,
struct rxrpc_connection *conn,
struct sk_buff *skb)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
- struct rxrpc_sock *rx;
struct rxrpc_call *call;
- u16 service_id = sp->hdr.serviceId;
_enter("");
- /* Get the socket providing the service */
- rx = rcu_dereference(local->service);
- if (rx && (service_id == rx->srx.srx_service ||
- service_id == rx->second_service))
- goto found_service;
-
- trace_rxrpc_abort(0, "INV", sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
- RX_INVALID_OPERATION, EOPNOTSUPP);
- skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
- skb->priority = RX_INVALID_OPERATION;
- _leave(" = NULL [service]");
- return NULL;
-
-found_service:
spin_lock(&rx->incoming_lock);
if (rx->sk.sk_state == RXRPC_SERVER_LISTEN_DISABLED ||
rx->sk.sk_state == RXRPC_CLOSE) {
@@ -371,7 +354,7 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
goto out;
}
- call = rxrpc_alloc_incoming_call(rx, local, conn, skb);
+ call = rxrpc_alloc_incoming_call(rx, local, peer, conn, skb);
if (!call) {
skb->mark = RXRPC_SKB_MARK_REJECT_BUSY;
_leave(" = NULL [busy]");
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 390ba50cfab4..b4438f98dc5c 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -69,10 +69,14 @@ struct rxrpc_connection *rxrpc_alloc_connection(gfp_t gfp)
* If successful, a pointer to the connection is returned, but no ref is taken.
* NULL is returned if there is no match.
*
+ * When searching for a service call, if we find a peer but no connection, we
+ * return that through *_peer in case we need to create a new service call.
+ *
* The caller must be holding the RCU read lock.
*/
struct rxrpc_connection *rxrpc_find_connection_rcu(struct rxrpc_local *local,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ struct rxrpc_peer **_peer)
{
struct rxrpc_connection *conn;
struct rxrpc_conn_proto k;
@@ -104,6 +108,7 @@ struct rxrpc_connection *rxrpc_find_connection_rcu(struct rxrpc_local *local,
peer = rxrpc_lookup_peer_rcu(local, &srx);
if (!peer)
goto not_found;
+ *_peer = peer;
conn = rxrpc_find_service_conn_rcu(peer, skb);
if (!conn || atomic_read(&conn->usage) == 0)
goto not_found;
diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
index a569e9e010d1..800f5b8a1baa 100644
--- a/net/rxrpc/input.c
+++ b/net/rxrpc/input.c
@@ -1128,7 +1128,8 @@ void rxrpc_data_ready(struct sock *udp_sk)
struct rxrpc_call *call = NULL;
struct rxrpc_skb_priv *sp;
struct rxrpc_local *local = udp_sk->sk_user_data;
- struct rxrpc_sock *rx;
+ struct rxrpc_peer *peer = NULL;
+ struct rxrpc_sock *rx = NULL;
struct sk_buff *skb;
unsigned int channel;
int ret, skew = 0;
@@ -1250,7 +1251,7 @@ void rxrpc_data_ready(struct sock *udp_sk)
}
}
- conn = rxrpc_find_connection_rcu(local, skb);
+ conn = rxrpc_find_connection_rcu(local, skb, &peer);
if (conn) {
if (sp->hdr.securityIndex != conn->security_ix)
goto wrong_security;
@@ -1339,7 +1340,7 @@ void rxrpc_data_ready(struct sock *udp_sk)
goto bad_message_unlock;
if (sp->hdr.seq != 1)
goto discard_unlock;
- call = rxrpc_new_incoming_call(local, conn, skb);
+ call = rxrpc_new_incoming_call(local, rx, peer, conn, skb);
if (!call) {
rcu_read_unlock();
goto reject_packet;
diff --git a/net/rxrpc/peer_object.c b/net/rxrpc/peer_object.c
index 1dc7648e3eff..70083e8fb6e5 100644
--- a/net/rxrpc/peer_object.c
+++ b/net/rxrpc/peer_object.c
@@ -124,11 +124,9 @@ static struct rxrpc_peer *__rxrpc_lookup_peer_rcu(
struct rxrpc_net *rxnet = local->rxnet;
hash_for_each_possible_rcu(rxnet->peer_hash, peer, hash_link, hash_key) {
- if (rxrpc_peer_cmp_key(peer, local, srx, hash_key) == 0) {
- if (atomic_read(&peer->usage) == 0)
- return NULL;
+ if (rxrpc_peer_cmp_key(peer, local, srx, hash_key) == 0 &&
+ atomic_read(&peer->usage) > 0)
return peer;
- }
}
return NULL;
@@ -299,34 +297,23 @@ static struct rxrpc_peer *rxrpc_create_peer(struct rxrpc_local *local,
}
/*
- * Set up a new incoming peer. The address is prestored in the preallocated
- * peer.
+ * Set up a new incoming peer. There shouldn't be any other matching peers
+ * since we've already done a search in the list from the non-reentrant context
+ * (the data_ready handler) that is the only place we can add new peers.
*/
-struct rxrpc_peer *rxrpc_lookup_incoming_peer(struct rxrpc_local *local,
- struct rxrpc_peer *prealloc)
+void rxrpc_new_incoming_peer(struct rxrpc_local *local, struct rxrpc_peer *peer)
{
- struct rxrpc_peer *peer;
struct rxrpc_net *rxnet = local->rxnet;
unsigned long hash_key;
- hash_key = rxrpc_peer_hash_key(local, &prealloc->srx);
- prealloc->local = local;
- rxrpc_init_peer(prealloc, hash_key);
+ hash_key = rxrpc_peer_hash_key(local, &peer->srx);
+ peer->local = local;
+ rxrpc_init_peer(peer, hash_key);
spin_lock(&rxnet->peer_hash_lock);
-
- /* Need to check that we aren't racing with someone else */
- peer = __rxrpc_lookup_peer_rcu(local, &prealloc->srx, hash_key);
- if (peer && !rxrpc_get_peer_maybe(peer))
- peer = NULL;
- if (!peer) {
- peer = prealloc;
- hash_add_rcu(rxnet->peer_hash, &peer->hash_link, hash_key);
- list_add_tail(&peer->keepalive_link, &rxnet->peer_keepalive_new);
- }
-
+ hash_add_rcu(rxnet->peer_hash, &peer->hash_link, hash_key);
+ list_add_tail(&peer->keepalive_link, &rxnet->peer_keepalive_new);
spin_unlock(&rxnet->peer_hash_lock);
- return peer;
}
/*
^ permalink raw reply related
* [PATCH net 7/8] rxrpc: Fix transport sockopts to get IPv4 errors on an IPv6 socket
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
It seems that enabling IPV6_RECVERR on an IPv6 socket doesn't also turn on
IP_RECVERR, so neither local errors nor ICMP-transported remote errors from
IPv4 peer addresses are returned to the AF_RXRPC protocol.
Make the sockopt setting code in rxrpc_open_socket() fall through from the
AF_INET6 case to the AF_INET case to turn on all the AF_INET options too in
the AF_INET6 case.
Fixes: f2aeed3a591f ("rxrpc: Fix error reception on AF_INET6 sockets")
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/local_object.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index 81de7d889ffa..94d234e9c685 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -135,10 +135,10 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
}
switch (local->srx.transport.family) {
- case AF_INET:
- /* we want to receive ICMP errors */
+ case AF_INET6:
+ /* we want to receive ICMPv6 errors */
opt = 1;
- ret = kernel_setsockopt(local->socket, SOL_IP, IP_RECVERR,
+ ret = kernel_setsockopt(local->socket, SOL_IPV6, IPV6_RECVERR,
(char *) &opt, sizeof(opt));
if (ret < 0) {
_debug("setsockopt failed");
@@ -146,19 +146,22 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
}
/* we want to set the don't fragment bit */
- opt = IP_PMTUDISC_DO;
- ret = kernel_setsockopt(local->socket, SOL_IP, IP_MTU_DISCOVER,
+ opt = IPV6_PMTUDISC_DO;
+ ret = kernel_setsockopt(local->socket, SOL_IPV6, IPV6_MTU_DISCOVER,
(char *) &opt, sizeof(opt));
if (ret < 0) {
_debug("setsockopt failed");
goto error;
}
- break;
- case AF_INET6:
+ /* Fall through and set IPv4 options too otherwise we don't get
+ * errors from IPv4 packets sent through the IPv6 socket.
+ */
+
+ case AF_INET:
/* we want to receive ICMP errors */
opt = 1;
- ret = kernel_setsockopt(local->socket, SOL_IPV6, IPV6_RECVERR,
+ ret = kernel_setsockopt(local->socket, SOL_IP, IP_RECVERR,
(char *) &opt, sizeof(opt));
if (ret < 0) {
_debug("setsockopt failed");
@@ -166,8 +169,8 @@ static int rxrpc_open_socket(struct rxrpc_local *local, struct net *net)
}
/* we want to set the don't fragment bit */
- opt = IPV6_PMTUDISC_DO;
- ret = kernel_setsockopt(local->socket, SOL_IPV6, IPV6_MTU_DISCOVER,
+ opt = IP_PMTUDISC_DO;
+ ret = kernel_setsockopt(local->socket, SOL_IP, IP_MTU_DISCOVER,
(char *) &opt, sizeof(opt));
if (ret < 0) {
_debug("setsockopt failed");
^ permalink raw reply related
* [PATCH net 8/8] rxrpc: Fix error distribution
From: David Howells @ 2018-09-28 10:10 UTC (permalink / raw)
To: netdev; +Cc: dhowells, linux-afs, linux-kernel
In-Reply-To: <153812939111.10469.179989653628053410.stgit@warthog.procyon.org.uk>
Fix error distribution by immediately delivering the errors to all the
affected calls rather than deferring them to a worker thread. The problem
with the latter is that retries and things can happen in the meantime when we
want to stop that sooner.
To this end:
(1) Stop the error distributor from removing calls from the error_targets
list so that peer->lock isn't needed to synchronise against other adds
and removals.
(2) Require the peer's error_targets list to be accessed with RCU, thereby
avoiding the need to take peer->lock over distribution.
(3) Don't attempt to affect a call's state if it is already marked complete.
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/trace/events/rxrpc.h | 4 +---
net/rxrpc/ar-internal.h | 5 -----
net/rxrpc/call_object.c | 2 +-
net/rxrpc/conn_client.c | 4 ++--
net/rxrpc/conn_object.c | 2 +-
net/rxrpc/peer_event.c | 46 ++++++++++--------------------------------
net/rxrpc/peer_object.c | 17 ----------------
7 files changed, 16 insertions(+), 64 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 196587b8f204..837393fa897b 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -56,7 +56,6 @@ enum rxrpc_peer_trace {
rxrpc_peer_new,
rxrpc_peer_processing,
rxrpc_peer_put,
- rxrpc_peer_queued_error,
};
enum rxrpc_conn_trace {
@@ -257,8 +256,7 @@ enum rxrpc_tx_point {
EM(rxrpc_peer_got, "GOT") \
EM(rxrpc_peer_new, "NEW") \
EM(rxrpc_peer_processing, "PRO") \
- EM(rxrpc_peer_put, "PUT") \
- E_(rxrpc_peer_queued_error, "QER")
+ E_(rxrpc_peer_put, "PUT")
#define rxrpc_conn_traces \
EM(rxrpc_conn_got, "GOT") \
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index c72686193d83..ef9554131434 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -288,7 +288,6 @@ struct rxrpc_peer {
struct hlist_node hash_link;
struct rxrpc_local *local;
struct hlist_head error_targets; /* targets for net error distribution */
- struct work_struct error_distributor;
struct rb_root service_conns; /* Service connections */
struct list_head keepalive_link; /* Link in net->peer_keepalive[] */
time64_t last_tx_at; /* Last time packet sent here */
@@ -299,8 +298,6 @@ struct rxrpc_peer {
unsigned int maxdata; /* data size (MTU - hdrsize) */
unsigned short hdrsize; /* header size (IP + UDP + RxRPC) */
int debug_id; /* debug ID for printks */
- int error_report; /* Net (+0) or local (+1000000) to distribute */
-#define RXRPC_LOCAL_ERROR_OFFSET 1000000
struct sockaddr_rxrpc srx; /* remote address */
/* calculated RTT cache */
@@ -1039,7 +1036,6 @@ void rxrpc_send_keepalive(struct rxrpc_peer *);
* peer_event.c
*/
void rxrpc_error_report(struct sock *);
-void rxrpc_peer_error_distributor(struct work_struct *);
void rxrpc_peer_add_rtt(struct rxrpc_call *, enum rxrpc_rtt_rx_trace,
rxrpc_serial_t, rxrpc_serial_t, ktime_t, ktime_t);
void rxrpc_peer_keepalive_worker(struct work_struct *);
@@ -1057,7 +1053,6 @@ void rxrpc_destroy_all_peers(struct rxrpc_net *);
struct rxrpc_peer *rxrpc_get_peer(struct rxrpc_peer *);
struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *);
void rxrpc_put_peer(struct rxrpc_peer *);
-void __rxrpc_queue_peer_error(struct rxrpc_peer *);
/*
* proc.c
diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 9486293fef5c..799f75b6900d 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -400,7 +400,7 @@ void rxrpc_incoming_call(struct rxrpc_sock *rx,
rcu_assign_pointer(conn->channels[chan].call, call);
spin_lock(&conn->params.peer->lock);
- hlist_add_head(&call->error_link, &conn->params.peer->error_targets);
+ hlist_add_head_rcu(&call->error_link, &conn->params.peer->error_targets);
spin_unlock(&conn->params.peer->lock);
_net("CALL incoming %d on CONN %d", call->debug_id, call->conn->debug_id);
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index f8f37188a932..8acf74fe24c0 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -710,8 +710,8 @@ int rxrpc_connect_call(struct rxrpc_call *call,
}
spin_lock_bh(&call->conn->params.peer->lock);
- hlist_add_head(&call->error_link,
- &call->conn->params.peer->error_targets);
+ hlist_add_head_rcu(&call->error_link,
+ &call->conn->params.peer->error_targets);
spin_unlock_bh(&call->conn->params.peer->lock);
out:
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index b4438f98dc5c..885dae829f4a 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -216,7 +216,7 @@ void rxrpc_disconnect_call(struct rxrpc_call *call)
call->peer->cong_cwnd = call->cong_cwnd;
spin_lock_bh(&conn->params.peer->lock);
- hlist_del_init(&call->error_link);
+ hlist_del_rcu(&call->error_link);
spin_unlock_bh(&conn->params.peer->lock);
if (rxrpc_is_client_call(call))
diff --git a/net/rxrpc/peer_event.c b/net/rxrpc/peer_event.c
index 4f9da2f51c69..f3e6fc670da2 100644
--- a/net/rxrpc/peer_event.c
+++ b/net/rxrpc/peer_event.c
@@ -23,6 +23,8 @@
#include "ar-internal.h"
static void rxrpc_store_error(struct rxrpc_peer *, struct sock_exterr_skb *);
+static void rxrpc_distribute_error(struct rxrpc_peer *, int,
+ enum rxrpc_call_completion);
/*
* Find the peer associated with an ICMP packet.
@@ -194,8 +196,6 @@ void rxrpc_error_report(struct sock *sk)
rcu_read_unlock();
rxrpc_free_skb(skb, rxrpc_skb_rx_freed);
- /* The ref we obtained is passed off to the work item */
- __rxrpc_queue_peer_error(peer);
_leave("");
}
@@ -205,6 +205,7 @@ void rxrpc_error_report(struct sock *sk)
static void rxrpc_store_error(struct rxrpc_peer *peer,
struct sock_exterr_skb *serr)
{
+ enum rxrpc_call_completion compl = RXRPC_CALL_NETWORK_ERROR;
struct sock_extended_err *ee;
int err;
@@ -255,7 +256,7 @@ static void rxrpc_store_error(struct rxrpc_peer *peer,
case SO_EE_ORIGIN_NONE:
case SO_EE_ORIGIN_LOCAL:
_proto("Rx Received local error { error=%d }", err);
- err += RXRPC_LOCAL_ERROR_OFFSET;
+ compl = RXRPC_CALL_LOCAL_ERROR;
break;
case SO_EE_ORIGIN_ICMP6:
@@ -264,48 +265,23 @@ static void rxrpc_store_error(struct rxrpc_peer *peer,
break;
}
- peer->error_report = err;
+ rxrpc_distribute_error(peer, err, compl);
}
/*
- * Distribute an error that occurred on a peer
+ * Distribute an error that occurred on a peer.
*/
-void rxrpc_peer_error_distributor(struct work_struct *work)
+static void rxrpc_distribute_error(struct rxrpc_peer *peer, int error,
+ enum rxrpc_call_completion compl)
{
- struct rxrpc_peer *peer =
- container_of(work, struct rxrpc_peer, error_distributor);
struct rxrpc_call *call;
- enum rxrpc_call_completion compl;
- int error;
-
- _enter("");
-
- error = READ_ONCE(peer->error_report);
- if (error < RXRPC_LOCAL_ERROR_OFFSET) {
- compl = RXRPC_CALL_NETWORK_ERROR;
- } else {
- compl = RXRPC_CALL_LOCAL_ERROR;
- error -= RXRPC_LOCAL_ERROR_OFFSET;
- }
- _debug("ISSUE ERROR %s %d", rxrpc_call_completions[compl], error);
-
- spin_lock_bh(&peer->lock);
-
- while (!hlist_empty(&peer->error_targets)) {
- call = hlist_entry(peer->error_targets.first,
- struct rxrpc_call, error_link);
- hlist_del_init(&call->error_link);
+ hlist_for_each_entry_rcu(call, &peer->error_targets, error_link) {
rxrpc_see_call(call);
-
- if (rxrpc_set_call_completion(call, compl, 0, -error))
+ if (call->state < RXRPC_CALL_COMPLETE &&
+ rxrpc_set_call_completion(call, compl, 0, -error))
rxrpc_notify_socket(call);
}
-
- spin_unlock_bh(&peer->lock);
-
- rxrpc_put_peer(peer);
- _leave("");
}
/*
diff --git a/net/rxrpc/peer_object.c b/net/rxrpc/peer_object.c
index 70083e8fb6e5..01a9febfa367 100644
--- a/net/rxrpc/peer_object.c
+++ b/net/rxrpc/peer_object.c
@@ -220,8 +220,6 @@ struct rxrpc_peer *rxrpc_alloc_peer(struct rxrpc_local *local, gfp_t gfp)
atomic_set(&peer->usage, 1);
peer->local = local;
INIT_HLIST_HEAD(&peer->error_targets);
- INIT_WORK(&peer->error_distributor,
- &rxrpc_peer_error_distributor);
peer->service_conns = RB_ROOT;
seqlock_init(&peer->service_conn_lock);
spin_lock_init(&peer->lock);
@@ -402,21 +400,6 @@ struct rxrpc_peer *rxrpc_get_peer_maybe(struct rxrpc_peer *peer)
return peer;
}
-/*
- * Queue a peer record. This passes the caller's ref to the workqueue.
- */
-void __rxrpc_queue_peer_error(struct rxrpc_peer *peer)
-{
- const void *here = __builtin_return_address(0);
- int n;
-
- n = atomic_read(&peer->usage);
- if (rxrpc_queue_work(&peer->error_distributor))
- trace_rxrpc_peer(peer, rxrpc_peer_queued_error, n, here);
- else
- rxrpc_put_peer(peer);
-}
-
/*
* Discard a peer record.
*/
^ permalink raw reply related
* RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
From: Ioana Ciocoi Radulescu @ 2018-09-28 10:20 UTC (permalink / raw)
To: Y.b. Lu, Andrew Lunn
Cc: devel@driverdev.osuosl.org, netdev@vger.kernel.org,
Richard Cochran, linux-kernel@vger.kernel.org, Greg Kroah-Hartman,
David S . Miller
In-Reply-To: <VI1PR0401MB223737CACE4E4064AE8325A4F8EC0@VI1PR0401MB2237.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: Y.b. Lu
> Sent: Friday, September 28, 2018 11:04 AM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
>
> Hi Andrew,
>
> Thanks a lot for your comments.
> Please see my comments inline.
>
> Best regards,
> Yangbo Lu
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu <yangbo.lu@nxp.com>
> > Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org;
> > netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;
> > David S . Miller <davem@davemloft.net>; Ioana Ciocoi Radulescu
> > <ruxandra.radulescu@nxp.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>
> > Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of
> staging/
> >
> > On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> > > This patch is to move DPAA2 PTP driver out of staging/ since the
> > > dpaa2-eth had been moved out.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > ---
> > > drivers/net/ethernet/freescale/Kconfig | 9 +--------
> > > drivers/net/ethernet/freescale/dpaa2/Kconfig | 15
> > +++++++++++++++
> > > drivers/net/ethernet/freescale/dpaa2/Makefile | 6 ++++--
> > > .../ethernet/freescale/dpaa2}/dprtc-cmd.h | 0
> > > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c | 0
> > > .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h | 0
> > > .../rtc => net/ethernet/freescale/dpaa2}/rtc.c | 0
> > > .../rtc => net/ethernet/freescale/dpaa2}/rtc.h | 0
> > > drivers/staging/fsl-dpaa2/Kconfig | 8 --------
> > > drivers/staging/fsl-dpaa2/Makefile | 1 -
> > > drivers/staging/fsl-dpaa2/rtc/Makefile | 7 -------
> > > 11 files changed, 20 insertions(+), 26 deletions(-) create mode
> > > 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
> > > rename drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%) rename
> > > drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc.c (100%) rename
> > > drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc.h (100%) rename
> > > drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c
> > > (100%) rename drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/rtc.h (100%)
> >
> > Hi Yangbo
> >
> > Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the name,
> > change it to ptp.[ch]. Also, some of the function names, and structures,
> > rtc_probe->ptp_probe, rtc_remove->ptp_remove, rtc_match_id_table->
> > ptp_match_id_table, etc.
>
> [Y.b. Lu] Indeed, it's odd and confusing...
> For dpaa2, all hardware resources are allocated and configured through the
> Management Complex (MC) portals.
> MC abstracts most of these resources as DPAA2 objects and exposes ABIs
> through which they can be configured and controlled.
> This ptp timer was named as rtc in MC firmware and APIs as you saw in
> dprtc.*.
> So initially I wrote this driver using rtc as name.
>
> No worries, let me change it in next version.
>
> >
> > ptp_dpaa2_adjfreq() probably should return err, not 0.
> > ptp_dpaa2_gettime() again does not return the error.
> > If fact, it seems like all the main functions ignore errors.
>
> [Y.b. Lu] Will fix the returns in next version.
>
> >
> > kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
>
> [Y.b. Lu] Will use devm_kzalloc() in next version.
>
> Can
> > ptp_dpaa2_caps be made const?
>
> [Y.b. Lu] Yes. Will change it in next version.
>
> > dpaa2_phc_index does not appear to be used.
>
> [Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of
> ethtool_ops.
>
> > dev_set_drvdata(dev, NULL); is not needed.
>
> [Y.b. Lu] Will remove it in next version.
>
> > Can rtc_drv be made const?
>
> [Y.b. Lu] Will use const in next version.
>
> > Is rtc.h used by anything other than rtc.c? It seems like it can be removed.
>
> [Y.b. Lu] Let me remove it in next version.
>
> >
> > It seems like there is a lot of code in dprtc.c which is unused. rtc.c does
> nothing
> > with interrupts for example. Do you plan to make use of this extra code? Or
> > can it be removed leaving just what is needed?
>
> [Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra code is
> being planed to be used.
Are there any interrupts associated to the real time clock module that will
actually be used by the driver? Also, I don't think the create/destroy functions
are meant to be used by the PTP kernel driver, even though MC exposes the
APIs for them.
Generally speaking, I think it's better to remove unused code from the current
driver and re-add it along with the feature actually using it.
>
> >
> > struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct seems
> very
> > odd. And it is not the only example.
>
> [Y.b. Lu] This should depended on MC firmware and APIs I think. Once the
> MC improves this, the APIs could be updated to fix this.
These structures map the command format expected by the MC firmware. I
agree that some of the command layouts are less than inspired, but I'm not
sure we can expect MC to "improve" them without a good reason, as this would
break backward compatibility.
I also want to bring up the question of where the dpaa2 ptp driver should be
located. The qoriq_ptp driver (which targets previous gen Freescale/NXP
architectures) is located in drivers/ptp. I'm not sure if the dpaa2 ptp driver should
be moved there as well or it's better suited for the currently proposed location.
Thanks,
Ioana
^ permalink raw reply
* Re: [PATCH v3 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Alexei Starovoitov @ 2018-09-28 10:25 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928100302.GB9018@castle.DHCP.thefacebook.com>
On Fri, Sep 28, 2018 at 11:03:03AM +0100, Roman Gushchin wrote:
> > > +
> > > + if (unlikely(map_flags & BPF_EXIST))
> > > + return -EINVAL;
> >
> > that should have been BPF_NOEXIST ?
>
> Yeah, or maybe even better s/&/!= ?
> It's probably better to require BPF_EXIST flag to update a cgroup storage?
> Agree? If so, let me fix this for both shared and per-cpu versions in
> a follow-up patch.
I think BPF_ANY is technically valid too.
If we were to require strict BPF_EXIST only, we'd need to fix stable too.
I'm fine with both (BPF_EXIST only and BPF_ANY|BPF_EXIST).
Daniel, what do you think?
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Alexei Starovoitov @ 2018-09-28 10:28 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928100817.GC9018@castle.DHCP.thefacebook.com>
On Fri, Sep 28, 2018 at 11:08:29AM +0100, Roman Gushchin wrote:
> > > + /* Some packets can be still in per-cpu cache, but not more than
> > > + * MAX_PERCPU_PACKETS.
> > > + */
> > > + packets = netcnt.packets;
> > > + for (cpu = 0; cpu < nproc; cpu++) {
> > > + if (percpu_netcnt[cpu].packets > 32) {
> >
> > pls use MAX_PERCPU_PACKETS in the above check.
> > could you also double check that if that #define is changed to 1k or so
> > the exact "!= 10000" check below still works as expected?
>
> Do you mean adding a new test with a different MAX_PERCPU_PACKETS?
good idea! If it's easy to compile the same source twice with different
MAX_PERCPU_PACKETS that would certainly make the test stronger.
Not sure how feasible though.
>
> >
> > > + printf("Unexpected percpu value: %llu\n",
> > > + percpu_netcnt[cpu].packets);
> > > + goto err;
> >
> > > + }
> > > +
> > > + packets += percpu_netcnt[cpu].packets;
> > > + }
> > > +
> > > + /* No packets should be lost */
> > > + if (packets != 10000) {
> > > + printf("Unexpected packet count: %lu\n", packets);
> > > + goto err;
> > > + }
> > > +
> > > + /* Let's check that bytes counter value is reasonable */
> > > + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
> >
> > since packet count is accurate why byte count would vary ?
>
> Tbh I'm not sure if the size of the packet here can vary depending
> on the environment. Is there a nice way to get the expected size?
ping packets should be fixed size depending on v4 vs v6.
If 'ping -6' is used, it will force ipv6.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Roman Gushchin @ 2018-09-28 10:37 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928102814.voj6bsuslxh6ms37@ast-mbp.dhcp.thefacebook.com>
On Fri, Sep 28, 2018 at 12:28:16PM +0200, Alexei Starovoitov wrote:
> On Fri, Sep 28, 2018 at 11:08:29AM +0100, Roman Gushchin wrote:
> > > > + /* Some packets can be still in per-cpu cache, but not more than
> > > > + * MAX_PERCPU_PACKETS.
> > > > + */
> > > > + packets = netcnt.packets;
> > > > + for (cpu = 0; cpu < nproc; cpu++) {
> > > > + if (percpu_netcnt[cpu].packets > 32) {
> > >
> > > pls use MAX_PERCPU_PACKETS in the above check.
> > > could you also double check that if that #define is changed to 1k or so
> > > the exact "!= 10000" check below still works as expected?
> >
> > Do you mean adding a new test with a different MAX_PERCPU_PACKETS?
>
> good idea! If it's easy to compile the same source twice with different
> MAX_PERCPU_PACKETS that would certainly make the test stronger.
> Not sure how feasible though.
>
> >
> > >
> > > > + printf("Unexpected percpu value: %llu\n",
> > > > + percpu_netcnt[cpu].packets);
> > > > + goto err;
> > >
> > > > + }
> > > > +
> > > > + packets += percpu_netcnt[cpu].packets;
> > > > + }
> > > > +
> > > > + /* No packets should be lost */
> > > > + if (packets != 10000) {
> > > > + printf("Unexpected packet count: %lu\n", packets);
> > > > + goto err;
> > > > + }
> > > > +
> > > > + /* Let's check that bytes counter value is reasonable */
> > > > + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
> > >
> > > since packet count is accurate why byte count would vary ?
> >
> > Tbh I'm not sure if the size of the packet here can vary depending
> > on the environment. Is there a nice way to get the expected size?
>
> ping packets should be fixed size depending on v4 vs v6.
> If 'ping -6' is used, it will force ipv6.
>
Are we ok to screw up kselftests on v4-only machines?
Alternatively, I can send 1 packet, get the size and check that all other
are of the same size.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 10/10] selftests/bpf: cgroup local storage-based network counters
From: Alexei Starovoitov @ 2018-09-28 10:40 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, Song Liu, linux-kernel, kernel-team, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20180928103717.GA12917@castle.DHCP.thefacebook.com>
On Fri, Sep 28, 2018 at 11:37:26AM +0100, Roman Gushchin wrote:
> > > > > +
> > > > > + /* Let's check that bytes counter value is reasonable */
> > > > > + if (netcnt.bytes < packets * 500 || netcnt.bytes > packets * 1500) {
> > > >
> > > > since packet count is accurate why byte count would vary ?
> > >
> > > Tbh I'm not sure if the size of the packet here can vary depending
> > > on the environment. Is there a nice way to get the expected size?
> >
> > ping packets should be fixed size depending on v4 vs v6.
> > If 'ping -6' is used, it will force ipv6.
> >
>
> Are we ok to screw up kselftests on v4-only machines?
we already did. Some of the selftests/bpf use ping -6 already.
^ 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