* Re: [PATCH] can: fix warning in bcm_connect/proc_register
From: Andrey Konovalov @ 2016-10-25 12:12 UTC (permalink / raw)
To: Cong Wang
Cc: Oliver Hartkopp, David Miller, linux-can,
Linux Kernel Network Developers, LKML, syzkaller,
Kostya Serebryany, Alexander Potapenko, Dmitry Vyukov,
Eric Dumazet
In-Reply-To: <CAM_iQpVmhH_9vQcTw+wP1qmPLOijZZrMZrq=_e4i5fUXYDON0Q@mail.gmail.com>
Hi Oliver,
I can confirm that your patch fixes the warnings for me.
Tested-by: Andrey Konovalov <andreyknvl@google.com>
On Mon, Oct 24, 2016 at 10:17 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Oct 24, 2016 at 1:10 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Oct 24, 2016 at 12:11 PM, Oliver Hartkopp
>> <socketcan@hartkopp.net> wrote:
>>> if (proc_dir) {
>>> /* unique socket address as filename */
>>> sprintf(bo->procname, "%lu", sock_i_ino(sk));
>>> bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
>>> proc_dir,
>>> &bcm_proc_fops, sk);
>>> + if (!bo->bcm_proc_read) {
>>> + ret = -ENOMEM;
>>> + goto fail;
>>> + }
>>
>> Well, I meant we need to call proc_create_data() once per socket,
>> so we need a check before proc_create_data() too.
>
> Hmm, bo->bound should guarantee it, so never mind, your patch
> looks fine.
^ permalink raw reply
* [PATCH v2] LSO feature added to Cadence GEM driver
From: Rafal Ozieblo @ 2016-10-25 12:05 UTC (permalink / raw)
To: Nicolas Ferre, Eric Dumazet, netdev, linux-kernel; +Cc: Rafal Ozieblo
In-Reply-To: <1477315088-28396-1-git-send-email-rafalo@cadence.com>
New Cadence GEM hardware support Large Segment Offload (LSO):
TCP segmentation offload (TSO) as well as UDP fragmentation
offload (UFO). Support for those features was added to the driver.
Signed-off-by: Rafal Ozieblo <rafalo@cadence.com>
---
Changed in v2:
macb_lso_check_compatibility() changed to macb_features_check()
(with little modifications) and bind to .ndo_features_check.
(after Eric Dumazet suggestion)
---
drivers/net/ethernet/cadence/macb.c | 142 +++++++++++++++++++++++++++++++++---
drivers/net/ethernet/cadence/macb.h | 14 ++++
2 files changed, 144 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index b32444a..30169fd 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -32,7 +32,9 @@
#include <linux/of_gpio.h>
#include <linux/of_mdio.h>
#include <linux/of_net.h>
-
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/tcp.h>
#include "macb.h"
#define MACB_RX_BUFFER_SIZE 128
@@ -53,10 +55,13 @@
| MACB_BIT(TXERR))
#define MACB_TX_INT_FLAGS (MACB_TX_ERR_FLAGS | MACB_BIT(TCOMP))
-#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1))
-#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
+/* Max length of transmit frame must be a multiple of 8 bytes */
+#define MACB_TX_LEN_ALIGN 8
+#define MACB_MAX_TX_LEN ((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
+#define GEM_MAX_TX_LEN ((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1) & ~((unsigned int)(MACB_TX_LEN_ALIGN - 1)))
#define GEM_MTU_MIN_SIZE 68
+#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
#define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0)
#define MACB_WOL_ENABLED (0x1 << 1)
@@ -1212,7 +1217,8 @@ static void macb_poll_controller(struct net_device *dev)
static unsigned int macb_tx_map(struct macb *bp,
struct macb_queue *queue,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ unsigned int hdrlen)
{
dma_addr_t mapping;
unsigned int len, entry, i, tx_head = queue->tx_head;
@@ -1220,14 +1226,27 @@ static unsigned int macb_tx_map(struct macb *bp,
struct macb_dma_desc *desc;
unsigned int offset, size, count = 0;
unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
- unsigned int eof = 1;
- u32 ctrl;
+ unsigned int eof = 1, mss_mfs = 0;
+ u32 ctrl, lso_ctrl = 0, seq_ctrl = 0;
+
+ /* LSO */
+ if (skb_shinfo(skb)->gso_size != 0) {
+ if (IPPROTO_UDP == (((struct iphdr *)skb_network_header(skb))->protocol))
+ /* UDP - UFO */
+ lso_ctrl = MACB_LSO_UFO_ENABLE;
+ else
+ /* TCP - TSO */
+ lso_ctrl = MACB_LSO_TSO_ENABLE;
+ }
/* First, map non-paged data */
len = skb_headlen(skb);
+
+ /* first buffer length */
+ size = hdrlen;
+
offset = 0;
while (len) {
- size = min(len, bp->max_tx_length);
entry = macb_tx_ring_wrap(tx_head);
tx_skb = &queue->tx_skb[entry];
@@ -1247,6 +1266,8 @@ static unsigned int macb_tx_map(struct macb *bp,
offset += size;
count++;
tx_head++;
+
+ size = min(len, bp->max_tx_length);
}
/* Then, map paged data from fragments */
@@ -1300,6 +1321,20 @@ static unsigned int macb_tx_map(struct macb *bp,
desc = &queue->tx_ring[entry];
desc->ctrl = ctrl;
+ if (lso_ctrl) {
+ if (lso_ctrl == MACB_LSO_UFO_ENABLE)
+ /* include header and FCS in value given to h/w */
+ mss_mfs = skb_shinfo(skb)->gso_size +
+ skb_transport_offset(skb) + 4;
+ else /* TSO */ {
+ mss_mfs = skb_shinfo(skb)->gso_size;
+ /* TCP Sequence Number Source Select
+ * can be set only for TSO
+ */
+ seq_ctrl = 0;
+ }
+ }
+
do {
i--;
entry = macb_tx_ring_wrap(i);
@@ -1314,6 +1349,16 @@ static unsigned int macb_tx_map(struct macb *bp,
if (unlikely(entry == (TX_RING_SIZE - 1)))
ctrl |= MACB_BIT(TX_WRAP);
+ /* First descriptor is header descriptor */
+ if (i == queue->tx_head) {
+ ctrl |= MACB_BF(TX_LSO, lso_ctrl);
+ ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
+ } else
+ /* Only set MSS/MFS on payload descriptors
+ * (second or later descriptor)
+ */
+ ctrl |= MACB_BF(MSS_MFS, mss_mfs);
+
/* Set TX buffer descriptor */
macb_set_addr(desc, tx_skb->mapping);
/* desc->addr must be visible to hardware before clearing
@@ -1339,6 +1384,43 @@ static unsigned int macb_tx_map(struct macb *bp,
return 0;
}
+static netdev_features_t macb_features_check(struct sk_buff *skb,
+ struct net_device *dev,
+ netdev_features_t features)
+{
+ unsigned int nr_frags, f;
+ unsigned int hdrlen;
+
+ /* Validate LSO compatibility */
+
+ /* there is only one buffer */
+ if (!skb_is_nonlinear(skb))
+ return features;
+
+ /* length of header */
+ hdrlen = skb_transport_offset(skb);
+ if (IPPROTO_TCP == (((struct iphdr *)skb_network_header(skb))->protocol))
+ hdrlen += tcp_hdrlen(skb);
+
+ /* For LSO:
+ * When software supplies two or more payload buffers all payload buffers
+ * apart from the last must be a multiple of 8 bytes in size.
+ */
+ if (!IS_ALIGNED(skb_headlen(skb) - hdrlen, MACB_TX_LEN_ALIGN))
+ return features & ~MACB_NETIF_LSO;
+
+ nr_frags = skb_shinfo(skb)->nr_frags;
+ /* No need to check last fragment */
+ nr_frags--;
+ for (f = 0; f < nr_frags; f++) {
+ const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
+
+ if (!IS_ALIGNED(skb_frag_size(frag), MACB_TX_LEN_ALIGN))
+ return features & ~MACB_NETIF_LSO;
+ }
+ return features;
+}
+
static inline int macb_clear_csum(struct sk_buff *skb)
{
/* no change for packets without checksum offloading */
@@ -1363,7 +1445,27 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
struct macb *bp = netdev_priv(dev);
struct macb_queue *queue = &bp->queues[queue_index];
unsigned long flags;
- unsigned int count, nr_frags, frag_size, f;
+ unsigned int desc_cnt, nr_frags, frag_size, f;
+ unsigned int is_lso = 0, is_udp, hdrlen;
+
+ is_lso = (skb_shinfo(skb)->gso_size != 0);
+
+ if (is_lso) {
+ is_udp = (IPPROTO_UDP == (((struct iphdr *)skb_network_header(skb))->protocol));
+
+ /* length of headers */
+ if (is_udp)
+ /* only queue eth + ip headers separately for UDP */
+ hdrlen = skb_transport_offset(skb);
+ else
+ hdrlen = skb_transport_offset(skb) + tcp_hdrlen(skb);
+ if (skb_headlen(skb) < hdrlen) {
+ netdev_err(bp->dev, "Error - LSO headers fragmented!!!\n");
+ /* if this is required, would need to copy to single buffer */
+ return NETDEV_TX_BUSY;
+ }
+ } else
+ hdrlen = min(skb_headlen(skb), bp->max_tx_length);
#if defined(DEBUG) && defined(VERBOSE_DEBUG)
netdev_vdbg(bp->dev,
@@ -1378,17 +1480,21 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
* socket buffer: skb fragments of jumbo frames may need to be
* split into many buffer descriptors.
*/
- count = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
+ if (is_lso && (skb_headlen(skb) > hdrlen))
+ /* extra header descriptor if also payload in first buffer */
+ desc_cnt = DIV_ROUND_UP((skb_headlen(skb) - hdrlen), bp->max_tx_length) + 1;
+ else
+ desc_cnt = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
nr_frags = skb_shinfo(skb)->nr_frags;
for (f = 0; f < nr_frags; f++) {
frag_size = skb_frag_size(&skb_shinfo(skb)->frags[f]);
- count += DIV_ROUND_UP(frag_size, bp->max_tx_length);
+ desc_cnt += DIV_ROUND_UP(frag_size, bp->max_tx_length);
}
spin_lock_irqsave(&bp->lock, flags);
/* This is a hard error, log it. */
- if (CIRC_SPACE(queue->tx_head, queue->tx_tail, TX_RING_SIZE) < count) {
+ if (CIRC_SPACE(queue->tx_head, queue->tx_tail, TX_RING_SIZE) < desc_cnt) {
netif_stop_subqueue(dev, queue_index);
spin_unlock_irqrestore(&bp->lock, flags);
netdev_dbg(bp->dev, "tx_head = %u, tx_tail = %u\n",
@@ -1396,13 +1502,19 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_BUSY;
}
+ if (is_lso) {
+ if (is_udp)
+ /* zero UDP checksum, not calculated by h/w for UFO */
+ udp_hdr(skb)->check = 0;
+ }
+
if (macb_clear_csum(skb)) {
dev_kfree_skb_any(skb);
goto unlock;
}
/* Map socket buffer for DMA transfer */
- if (!macb_tx_map(bp, queue, skb)) {
+ if (!macb_tx_map(bp, queue, skb, hdrlen)) {
dev_kfree_skb_any(skb);
goto unlock;
}
@@ -2298,6 +2410,7 @@ static const struct net_device_ops macb_netdev_ops = {
.ndo_poll_controller = macb_poll_controller,
#endif
.ndo_set_features = macb_set_features,
+ .ndo_features_check = macb_features_check,
};
/* Configure peripheral capabilities according to device tree
@@ -2501,6 +2614,11 @@ static int macb_init(struct platform_device *pdev)
/* Set features */
dev->hw_features = NETIF_F_SG;
+
+ /* Check LSO capability */
+ if (GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
+ dev->hw_features |= MACB_NETIF_LSO;
+
/* Checksum offload is only available on gem with packet buffer */
if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8bed4b5..534aded 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -382,6 +382,10 @@
#define GEM_TX_PKT_BUFF_OFFSET 21
#define GEM_TX_PKT_BUFF_SIZE 1
+/* Bitfields in DCFG6. */
+#define GEM_PBUF_LSO_OFFSET 27
+#define GEM_PBUF_LSO_SIZE 1
+
/* Constants for CLK */
#define MACB_CLK_DIV8 0
#define MACB_CLK_DIV16 1
@@ -414,6 +418,10 @@
#define MACB_CAPS_SG_DISABLED 0x40000000
#define MACB_CAPS_MACB_IS_GEM 0x80000000
+/* LSO settings */
+#define MACB_LSO_UFO_ENABLE 0x01
+#define MACB_LSO_TSO_ENABLE 0x02
+
/* Bit manipulation macros */
#define MACB_BIT(name) \
(1 << MACB_##name##_OFFSET)
@@ -545,6 +553,12 @@ struct macb_dma_desc {
#define MACB_TX_LAST_SIZE 1
#define MACB_TX_NOCRC_OFFSET 16
#define MACB_TX_NOCRC_SIZE 1
+#define MACB_MSS_MFS_OFFSET 16
+#define MACB_MSS_MFS_SIZE 14
+#define MACB_TX_LSO_OFFSET 17
+#define MACB_TX_LSO_SIZE 2
+#define MACB_TX_TCP_SEQ_SRC_OFFSET 19
+#define MACB_TX_TCP_SEQ_SRC_SIZE 1
#define MACB_TX_BUF_EXHAUSTED_OFFSET 27
#define MACB_TX_BUF_EXHAUSTED_SIZE 1
#define MACB_TX_UNDERRUN_OFFSET 28
--
2.4.5
^ permalink raw reply related
* Re: question about function igmp_stop_timer() in net/ipv4/igmp.c
From: Dongpo Li @ 2016-10-25 11:50 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20161025073913.GA20223@lunn.ch>
On 2016/10/25 15:39, Andrew Lunn wrote:
> On Tue, Oct 25, 2016 at 09:13:54AM +0800, Dongpo Li wrote:
>> Hi Andrew,
>>
>> On 2016/10/24 23:32, Andrew Lunn wrote:
>>> On Mon, Oct 24, 2016 at 07:50:12PM +0800, Dongpo Li wrote:
>>>> Hello
>>>>
>>>> We encountered a multicast problem when two set-top box(STB) join the same multicast group and leave.
>>>> The two boxes can join the same multicast group
>>>> but only one box can send the IGMP leave group message when leave,
>>>> the other box does not send the IGMP leave message.
>>>> Our boxes use the IGMP version 2.
>>>>
>>>> I added some debug info and found the whole procedure is like this:
>>>> (1) Box A joins the multicast group 225.1.101.145 and send the IGMP v2 membership report(join group).
>>>> (2) Box B joins the same multicast group 225.1.101.145 and also send the IGMP v2 membership report(join group).
>>>> (3) Box A receives the IGMP membership report from Box B and kernel calls igmp_heard_report().
>>>> This function will call igmp_stop_timer(im).
>>>> In function igmp_stop_timer(im), it tries to delete IGMP timer and does the following:
>>>> im->tm_running = 0;
>>>> im->reporter = 0;
>>>> (4) Box A leaves the multicast group 225.1.101.145 and kernel calls
>>>> ip_mc_leave_group -> ip_mc_dec_group -> igmp_group_dropped.
>>>> But in function igmp_group_dropped(), the im->reporter is 0, so the kernel does not send the IGMP leave message.
>>>
>>> RFC 2236 says:
>>>
>>> 2. Introduction
>>>
>>> The Internet Group Management Protocol (IGMP) is used by IP hosts to
>>> report their multicast group memberships to any immediately-
>>> neighboring multicast routers.
>>>
>>> Are Box A or B multicast routers?
>> Thank you for your comments.
>> Both Box A and B are IP hosts, not multicast routers.
>> And the RFC says: IGMP is used by "IP hosts" to report their multicast group membership.
>
> They report their membership to gateways, not to each other. The
> gateway will then arrange for multicast traffic for the group from
> other subnets to be forwarded to this subnet. You don't need IGMP to
> receive local traffic.
>
> Also, this timer is to do with responding to IGMP querier, typically
> the multicast gateway. The querier keeps track of if a group is in use
> within a subnet. It listens to group joins and optionally leaves. It
> also periodically sends out IGMP querier requests, asking who is
> interested in what groups. The hosts use a random delay before
> answering, and if some other hosts replies about a group they are a
> member of, they don't send a response themselves. One is enough.
>
Of course, this timer is to do with responding to IGMP querier.
I mean, if the timer has expired, the host must have sent the IGMP responding message,
maybe IGMP join group message or responding router's IGMP querier.
So the function igmp_stop_timer() should not set im->reporter to 0 if the timer has expired.
Otherwise the host can't send IGMP leave message when it leaves the group because the im->reporter is 0.
Regards,
Dongpo
.
^ permalink raw reply
* Re: [PATCH 3/5] genetlink: statically initialize families
From: Johannes Berg @ 2016-10-25 11:38 UTC (permalink / raw)
To: David Laight, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB0209D95@AcuExch.aculab.com>
On Tue, 2016-10-25 at 11:25 +0000, David Laight wrote:
> > Actually, with the new system where it's not const, I could even
> > split this up and submit per subsystem, i.e. the fourth patch
> > doesn't depend on it. I thought it would, since I wanted to make it
> > const, but since I failed it doesn't actually have that dependency.
>
> Actually, why aren't the structures 'const' ?
>
> You could use a #define to set the .ops and .n_ops fields.
> (and maybe .module = THIS_MODULE as well).
This stuff isn't the problem - after this patch these are of course
statically initialized and const.
The problem is that the struct members family->id, family->mcgrp_offset
and family->attrbuf, are only determined at genl_register_family().
I considered simply moving them into a new struct, that contains just
those along with a pointer to the family, but then I have essentially
two choices:
1) look up the registration struct by the family every time I need the
family ID, which is all the time; that would be rather inefficient
2) change *all* genetlink code to not pass the family but rather pass a
pointer returned by genl_register_family(); that's a massive change
So on the whole, I decided that __ro_after_init was entirely reasonable
and then even this patch isn't really necessary, but since I had it
anyway it still seemed to make sense, even if I had to add all those
forward declarations.
johannes
^ permalink raw reply
* RE: [PATCH 3/5] genetlink: statically initialize families
From: David Laight @ 2016-10-25 11:25 UTC (permalink / raw)
To: 'Johannes Berg', netdev@vger.kernel.org
In-Reply-To: <1477313733.4085.11.camel@sipsolutions.net>
From: Johannes Berg
> Sent: 24 October 2016 13:56
> On Mon, 2016-10-24 at 14:40 +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > Instead of providing macros/inline functions to initialize
> > the families, make all users initialize them statically and
> > get rid of the macros.
> >
> > This reduces the kernel code size by about 1.6k on x86-64
> > (with allyesconfig).
>
> Actually, with the new system where it's not const, I could even split
> this up and submit per subsystem, i.e. the fourth patch doesn't depend
> on it. I thought it would, since I wanted to make it const, but since I
> failed it doesn't actually have that dependency.
Actually, why aren't the structures 'const' ?
You could use a #define to set the .ops and .n_ops fields.
(and maybe .module = THIS_MODULE as well).
David
^ permalink raw reply
* [PATCH] net caif: insert missing spaces in pr_* messages and unbreak multi-line strings
From: Colin King @ 2016-10-25 11:18 UTC (permalink / raw)
To: Dmitry Tarnyagin, David S . Miller, netdev; +Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Some of the pr_* messages are missing spaces, so insert these and also
unbreak multi-line literal strings in pr_* messages
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/caif/cfcnfg.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index fa39fc2..273cb07 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -390,8 +390,7 @@ cfcnfg_linkup_rsp(struct cflayer *layer, u8 channel_id, enum cfctrl_srv serv,
rcu_read_lock();
if (adapt_layer == NULL) {
- pr_debug("link setup response but no client exist,"
- "send linkdown back\n");
+ pr_debug("link setup response but no client exist, send linkdown back\n");
cfctrl_linkdown_req(cnfg->ctrl, channel_id, NULL);
goto unlock;
}
@@ -401,8 +400,7 @@ cfcnfg_linkup_rsp(struct cflayer *layer, u8 channel_id, enum cfctrl_srv serv,
phyinfo = cfcnfg_get_phyinfo_rcu(cnfg, phyid);
if (phyinfo == NULL) {
- pr_err("ERROR: Link Layer Device disappeared"
- "while connecting\n");
+ pr_err("ERROR: Link Layer Device disappeared while connecting\n");
goto unlock;
}
@@ -436,8 +434,7 @@ cfcnfg_linkup_rsp(struct cflayer *layer, u8 channel_id, enum cfctrl_srv serv,
servicel = cfdbgl_create(channel_id, &phyinfo->dev_info);
break;
default:
- pr_err("Protocol error. Link setup response "
- "- unknown channel type\n");
+ pr_err("Protocol error. Link setup response - unknown channel type\n");
goto unlock;
}
if (!servicel)
--
2.9.3
^ permalink raw reply related
* payment Compensation
From: Kouame Ganzrin Hugues. @ 2016-10-25 11:11 UTC (permalink / raw)
To: undisclosed-recipients
In-Reply-To: <612074412.67417690.1477393743015.JavaMail.zimbra@ciudad.com.ar>
Scam Victim Compensation
You are receiving this message from Economic and Financial Crimes
Commission (EFCC) in alliance with economic community of West African
states (ECOWAS). We have been working towards the eradication of
fraudsters and scam Artists in Western part of Africa With the help of
United States Government and the United Nations. We have been able to track
down so many of this scam artist in various parts of African countries and
they are all in our custody.
We have been able to recover so much money from these scam artists. The
United Nation Anti-crime commission and the United State Government have
ordered the money recovered from the Scammers to be shared among 100 Lucky
people around the globe. This email is been directed to you because your
email address was found in one of the scam Artists file and computer hard
disk in our custody here. You are therefore being compensated with $500,000
Five Hundred Thousand Dollars. We have also arrested all those who claim to
be barristers, bank officials, Lottery Agents who has money for such funds
which do not exist.
Since your name appeared among the lucky beneficiaries who will receive a
compensation of $1.500, 000 One Million Five Hundred Thousand Dollars, we
have arranged your payment through Bank draft check. So feel free to
contact the delivery company for the processing delivery of your bank draft
check. DHL Delivery Company contact Details
Name: Dr Albert Godwin
Email: dhlcompeny1234@gmail.com
Phone: +229 688-669-49
Best Regard,
Mr. Kouame Ganzrin Hugues.
^ permalink raw reply
* Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Jamal Hadi Salim @ 2016-10-25 11:04 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Xin Long, netdev@vger.kernel.org, Vlad Yasevich, Daniel Borkmann,
David Miller, linux-sctp@vger.kernel.org, Michael Tuexen,
Eric Dumazet, Brenda Butler, gabor
In-Reply-To: <20161025103416.GG2958@localhost.localdomain>
On 16-10-25 06:34 AM, Marcelo Ricardo Leitner wrote:
> On Tue, Oct 25, 2016 at 05:05:41PM +0800, Xin Long wrote:
>>>> in case [1], user can't see the ENOMEM, ENOMEM is more like
>
> Thing is, it may lead to duplicate messages in Application layer, as the
> msg that was errored out may have been actually queued and later
> retransmitted.
>
> That's why I said the recovery steps from this depends on the
> application on top of SCTP, if it can handle such duplicate messages or
> not.
Yes, I was worried about duplicate messages.
Which is a bug on SCTP implementation on Linux, unfortunately. IOW,
transport should take care of duplicates - not the app.
i.e any app change is a workaround which will be unnecessary in newer
kernels.
cheers,
jamal
^ permalink raw reply
* Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Marcelo Ricardo Leitner @ 2016-10-25 10:34 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Xin Long, netdev@vger.kernel.org, Vlad Yasevich, Daniel Borkmann,
David Miller, linux-sctp@vger.kernel.org, Michael Tuexen,
Eric Dumazet, Brenda Butler, gabor
In-Reply-To: <CADvbK_eEaZBDXfY6D5dqRq3WRSgxvcHUxbq6JPnSrZ6Zpkghrw@mail.gmail.com>
On Tue, Oct 25, 2016 at 05:05:41PM +0800, Xin Long wrote:
> >> in case [1], user can't see the ENOMEM, ENOMEM is more like
> >> a internal err.
> >>
> >
> > Still not clear. Are you saying, say an old kernel like 3.11 would
> > not return the user ENOMEN for the use case[1] you fixed? I am not
> > talking post your fix.
> Sorry for confusing you.
>
> 3.11 would return the user ENOMEN for the use case[1].
> but this behavior is incorrect, it's not consistent with tcp.
>
> >
> >> in case [2], user will got the ENOMEM, they should resend this msg,
> >> It's the the general case mentioned-above
> >>
> >
> > I am trying to see if we can avoid backporting this fix to 3.11.
> > In [1], is ENOMEM propagated to user space (dont talk about your
> > fix, I mean pre-your-fix).
> yes, in [1], pre-my-fix, ENOMEM is propagated to user space.
>
> >
> >
> >> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
> >> also may fail to alloc skb, but it doesn't return any err to user, just
> >> like
> >> sctp_packet_transmit. That's why I don't think we should change something
> >> in manpage, as here sctp is consistent with tcp now.
> >>
> >> make sense ?
> >
> >
> > No ;-> The manpage is bad. Go look at it. In the case of ENOBUFS or
> > EMSGSIZE it is clear what needs to be done.
> > If the answer is _on ENOMEM_ user must resend then thats what we need
> > to say.
> yes, on ENOMEM user must resend if he want send out this msg successfully.
Thing is, it may lead to duplicate messages in Application layer, as the
msg that was errored out may have been actually queued and later
retransmitted.
That's why I said the recovery steps from this depends on the
application on top of SCTP, if it can handle such duplicate messages or
not.
Marcelo
^ permalink raw reply
* Re: [PATCH net-next] ibmveth: calculate correct gso_size and set gso_type
From: Marcelo Ricardo Leitner @ 2016-10-25 10:31 UTC (permalink / raw)
To: Jon Maxwell
Cc: tlfalcon, benh, paulus, mpe, davem, tom, jarod, hofrat, netdev,
linuxppc-dev, linux-kernel, jmaxwell
In-Reply-To: <1477372421-11656-1-git-send-email-jmaxwell37@gmail.com>
On Tue, Oct 25, 2016 at 04:13:41PM +1100, Jon Maxwell wrote:
> We recently encountered a bug where a few customers using ibmveth on the
> same LPAR hit an issue where a TCP session hung when large receive was
> enabled. Closer analysis revealed that the session was stuck because the
> one side was advertising a zero window repeatedly.
>
> We narrowed this down to the fact the ibmveth driver did not set gso_size
> which is translated by TCP into the MSS later up the stack. The MSS is
> used to calculate the TCP window size and as that was abnormally large,
> it was calculating a zero window, even although the sockets receive buffer
> was completely empty.
>
> We were able to reproduce this and worked with IBM to fix this. Thanks Tom
> and Marcelo for all your help and review on this.
>
> The patch fixes both our internal reproduction tests and our customers tests.
>
> Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
> ---
> drivers/net/ethernet/ibm/ibmveth.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 29c05d0..3028c33 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1182,6 +1182,8 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> int frames_processed = 0;
> unsigned long lpar_rc;
> struct iphdr *iph;
> + bool large_packet = 0;
> + u16 hdr_len = ETH_HLEN + sizeof(struct tcphdr);
Compiler may optmize this, but maybe move hdr_len to [*] ?
>
> restart_poll:
> while (frames_processed < budget) {
> @@ -1236,10 +1238,27 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> iph->check = 0;
> iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
> adapter->rx_large_packets++;
> + large_packet = 1;
> }
> }
> }
>
> + if (skb->len > netdev->mtu) {
[*]
> + iph = (struct iphdr *)skb->data;
> + if (be16_to_cpu(skb->protocol) == ETH_P_IP && iph->protocol == IPPROTO_TCP) {
The if line above is too long, should be broken in two.
> + hdr_len += sizeof(struct iphdr);
> + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> + skb_shinfo(skb)->gso_size = netdev->mtu - hdr_len;
> + } else if (be16_to_cpu(skb->protocol) == ETH_P_IPV6 &&
> + iph->protocol == IPPROTO_TCP) {
^
And this one should start 3 spaces later, right below be16_....
Marcelo
> + hdr_len += sizeof(struct ipv6hdr);
> + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> + skb_shinfo(skb)->gso_size = netdev->mtu - hdr_len;
> + }
> + if (!large_packet)
> + adapter->rx_large_packets++;
> + }
> +
> napi_gro_receive(napi, skb); /* send it up */
>
> netdev->stats.rx_packets++;
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH v7 4/6] net: filter: run cgroup eBPF ingress programs
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun, daniel, ast
Cc: davem, kafai, fw, pablo, harald, netdev, sargun, cgroups,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel@zonque.org>
If the cgroup associated with the receiving socket has an eBPF
programs installed, run them from sk_filter_trim_cap().
eBPF programs used in this context are expected to either return 1 to
let the packet pass, or != 1 to drop them. The programs have access to
the skb through bpf_skb_load_bytes(), and the payload starts at the
network headers (L3).
Note that cgroup_bpf_run_filter() is stubbed out as static inline nop
for !CONFIG_CGROUP_BPF, and is otherwise guarded by a static key if
the feature is unused.
Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index e3813d6..bd6eebe 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -78,6 +78,10 @@ int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap)
if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC))
return -ENOMEM;
+ err = cgroup_bpf_run_filter(sk, skb, BPF_CGROUP_INET_INGRESS);
+ if (err)
+ return err;
+
err = security_sock_rcv_skb(sk, skb);
if (err)
return err;
--
2.7.4
^ permalink raw reply related
* [PATCH v7 1/6] bpf: add new prog type for cgroup socket filtering
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun, daniel, ast
Cc: davem, kafai, fw, pablo, harald, netdev, sargun, cgroups,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel@zonque.org>
This program type is similar to BPF_PROG_TYPE_SOCKET_FILTER, except that
it does not allow BPF_LD_[ABS|IND] instructions and hooks up the
bpf_skb_load_bytes() helper.
Programs of this type will be attached to cgroups for network filtering
and accounting.
Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 9 +++++++++
net/core/filter.c | 23 +++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f09c70b..1f3e6f1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -96,8 +96,17 @@ enum bpf_prog_type {
BPF_PROG_TYPE_TRACEPOINT,
BPF_PROG_TYPE_XDP,
BPF_PROG_TYPE_PERF_EVENT,
+ BPF_PROG_TYPE_CGROUP_SKB,
};
+enum bpf_attach_type {
+ BPF_CGROUP_INET_INGRESS,
+ BPF_CGROUP_INET_EGRESS,
+ __MAX_BPF_ATTACH_TYPE
+};
+
+#define MAX_BPF_ATTACH_TYPE __MAX_BPF_ATTACH_TYPE
+
#define BPF_PSEUDO_MAP_FD 1
/* flags for BPF_MAP_UPDATE_ELEM command */
diff --git a/net/core/filter.c b/net/core/filter.c
index 00351cd..e3813d6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2576,6 +2576,17 @@ xdp_func_proto(enum bpf_func_id func_id)
}
}
+static const struct bpf_func_proto *
+cg_skb_func_proto(enum bpf_func_id func_id)
+{
+ switch (func_id) {
+ case BPF_FUNC_skb_load_bytes:
+ return &bpf_skb_load_bytes_proto;
+ default:
+ return sk_filter_func_proto(func_id);
+ }
+}
+
static bool __is_valid_access(int off, int size, enum bpf_access_type type)
{
if (off < 0 || off >= sizeof(struct __sk_buff))
@@ -2938,6 +2949,12 @@ static const struct bpf_verifier_ops xdp_ops = {
.convert_ctx_access = xdp_convert_ctx_access,
};
+static const struct bpf_verifier_ops cg_skb_ops = {
+ .get_func_proto = cg_skb_func_proto,
+ .is_valid_access = sk_filter_is_valid_access,
+ .convert_ctx_access = sk_filter_convert_ctx_access,
+};
+
static struct bpf_prog_type_list sk_filter_type __read_mostly = {
.ops = &sk_filter_ops,
.type = BPF_PROG_TYPE_SOCKET_FILTER,
@@ -2958,12 +2975,18 @@ static struct bpf_prog_type_list xdp_type __read_mostly = {
.type = BPF_PROG_TYPE_XDP,
};
+static struct bpf_prog_type_list cg_skb_type __read_mostly = {
+ .ops = &cg_skb_ops,
+ .type = BPF_PROG_TYPE_CGROUP_SKB,
+};
+
static int __init register_sk_filter_ops(void)
{
bpf_register_prog_type(&sk_filter_type);
bpf_register_prog_type(&sched_cls_type);
bpf_register_prog_type(&sched_act_type);
bpf_register_prog_type(&xdp_type);
+ bpf_register_prog_type(&cg_skb_type);
return 0;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v7 6/6] samples: bpf: add userspace example for attaching eBPF programs to cgroups
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun-b10kYP2dOMg, daniel-FeC+5ew28dpmcu3hnIyYJQ,
ast-b10kYP2dOMg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Add a simple userpace program to demonstrate the new API to attach eBPF
programs to cgroups. This is what it does:
* Create arraymap in kernel with 4 byte keys and 8 byte values
* Load eBPF program
The eBPF program accesses the map passed in to store two pieces of
information. The number of invocations of the program, which maps
to the number of packets received, is stored to key 0. Key 1 is
incremented on each iteration by the number of bytes stored in
the skb.
* Detach any eBPF program previously attached to the cgroup
* Attach the new program to the cgroup using BPF_PROG_ATTACH
* Once a second, read map[0] and map[1] to see how many bytes and
packets were seen on any socket of tasks in the given cgroup.
The program takes a cgroup path as 1st argument, and either "ingress"
or "egress" as 2nd. Optionally, "drop" can be passed as 3rd argument,
which will make the generated eBPF program return 0 instead of 1, so
the kernel will drop the packet.
libbpf gained two new wrappers for the new syscall commands.
Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Acked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
samples/bpf/Makefile | 2 +
samples/bpf/libbpf.c | 21 ++++++
samples/bpf/libbpf.h | 3 +
samples/bpf/test_cgrp2_attach.c | 147 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 173 insertions(+)
create mode 100644 samples/bpf/test_cgrp2_attach.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 12b7304..e4cdc74 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -22,6 +22,7 @@ hostprogs-y += spintest
hostprogs-y += map_perf_test
hostprogs-y += test_overhead
hostprogs-y += test_cgrp2_array_pin
+hostprogs-y += test_cgrp2_attach
hostprogs-y += xdp1
hostprogs-y += xdp2
hostprogs-y += test_current_task_under_cgroup
@@ -49,6 +50,7 @@ spintest-objs := bpf_load.o libbpf.o spintest_user.o
map_perf_test-objs := bpf_load.o libbpf.o map_perf_test_user.o
test_overhead-objs := bpf_load.o libbpf.o test_overhead_user.o
test_cgrp2_array_pin-objs := libbpf.o test_cgrp2_array_pin.o
+test_cgrp2_attach-objs := libbpf.o test_cgrp2_attach.o
xdp1-objs := bpf_load.o libbpf.o xdp1_user.o
# reuse xdp1 source intentionally
xdp2-objs := bpf_load.o libbpf.o xdp1_user.o
diff --git a/samples/bpf/libbpf.c b/samples/bpf/libbpf.c
index 9969e35..9ce707b 100644
--- a/samples/bpf/libbpf.c
+++ b/samples/bpf/libbpf.c
@@ -104,6 +104,27 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
}
+int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type)
+{
+ union bpf_attr attr = {
+ .target_fd = target_fd,
+ .attach_bpf_fd = prog_fd,
+ .attach_type = type,
+ };
+
+ return syscall(__NR_bpf, BPF_PROG_ATTACH, &attr, sizeof(attr));
+}
+
+int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
+{
+ union bpf_attr attr = {
+ .target_fd = target_fd,
+ .attach_type = type,
+ };
+
+ return syscall(__NR_bpf, BPF_PROG_DETACH, &attr, sizeof(attr));
+}
+
int bpf_obj_pin(int fd, const char *pathname)
{
union bpf_attr attr = {
diff --git a/samples/bpf/libbpf.h b/samples/bpf/libbpf.h
index ac6edb6..d0a799a 100644
--- a/samples/bpf/libbpf.h
+++ b/samples/bpf/libbpf.h
@@ -15,6 +15,9 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
const struct bpf_insn *insns, int insn_len,
const char *license, int kern_version);
+int bpf_prog_attach(int prog_fd, int attachable_fd, enum bpf_attach_type type);
+int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
+
int bpf_obj_pin(int fd, const char *pathname);
int bpf_obj_get(const char *pathname);
diff --git a/samples/bpf/test_cgrp2_attach.c b/samples/bpf/test_cgrp2_attach.c
new file mode 100644
index 0000000..63ef208
--- /dev/null
+++ b/samples/bpf/test_cgrp2_attach.c
@@ -0,0 +1,147 @@
+/* eBPF example program:
+ *
+ * - Creates arraymap in kernel with 4 bytes keys and 8 byte values
+ *
+ * - Loads eBPF program
+ *
+ * The eBPF program accesses the map passed in to store two pieces of
+ * information. The number of invocations of the program, which maps
+ * to the number of packets received, is stored to key 0. Key 1 is
+ * incremented on each iteration by the number of bytes stored in
+ * the skb.
+ *
+ * - Detaches any eBPF program previously attached to the cgroup
+ *
+ * - Attaches the new program to a cgroup using BPF_PROG_ATTACH
+ *
+ * - Every second, reads map[0] and map[1] to see how many bytes and
+ * packets were seen on any socket of tasks in the given cgroup.
+ */
+
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <string.h>
+#include <unistd.h>
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+
+#include <linux/bpf.h>
+
+#include "libbpf.h"
+
+enum {
+ MAP_KEY_PACKETS,
+ MAP_KEY_BYTES,
+};
+
+static int prog_load(int map_fd, int verdict)
+{
+ struct bpf_insn prog[] = {
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_1), /* save r6 so it's not clobbered by BPF_CALL */
+
+ /* Count packets */
+ BPF_MOV64_IMM(BPF_REG_0, MAP_KEY_PACKETS), /* r0 = 0 */
+ BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp - 4) = r0 */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp - 4 */
+ BPF_LD_MAP_FD(BPF_REG_1, map_fd), /* load map fd to r1 */
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_1, 1), /* r1 = 1 */
+ BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+
+ /* Count bytes */
+ BPF_MOV64_IMM(BPF_REG_0, MAP_KEY_BYTES), /* r0 = 1 */
+ BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_0, -4), /* *(u32 *)(fp - 4) = r0 */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), /* r2 = fp - 4 */
+ BPF_LD_MAP_FD(BPF_REG_1, map_fd),
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
+ BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_6, offsetof(struct __sk_buff, len)), /* r1 = skb->len */
+ BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
+
+ BPF_MOV64_IMM(BPF_REG_0, verdict), /* r0 = verdict */
+ BPF_EXIT_INSN(),
+ };
+
+ return bpf_prog_load(BPF_PROG_TYPE_CGROUP_SKB,
+ prog, sizeof(prog), "GPL", 0);
+}
+
+static int usage(const char *argv0)
+{
+ printf("Usage: %s <cg-path> <egress|ingress> [drop]\n", argv0);
+ return EXIT_FAILURE;
+}
+
+int main(int argc, char **argv)
+{
+ int cg_fd, map_fd, prog_fd, key, ret;
+ long long pkt_cnt, byte_cnt;
+ enum bpf_attach_type type;
+ int verdict = 1;
+
+ if (argc < 3)
+ return usage(argv[0]);
+
+ if (strcmp(argv[2], "ingress") == 0)
+ type = BPF_CGROUP_INET_INGRESS;
+ else if (strcmp(argv[2], "egress") == 0)
+ type = BPF_CGROUP_INET_EGRESS;
+ else
+ return usage(argv[0]);
+
+ if (argc > 3 && strcmp(argv[3], "drop") == 0)
+ verdict = 0;
+
+ cg_fd = open(argv[1], O_DIRECTORY | O_RDONLY);
+ if (cg_fd < 0) {
+ printf("Failed to open cgroup path: '%s'\n", strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY,
+ sizeof(key), sizeof(byte_cnt),
+ 256, 0);
+ if (map_fd < 0) {
+ printf("Failed to create map: '%s'\n", strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ prog_fd = prog_load(map_fd, verdict);
+ printf("Output from kernel verifier:\n%s\n-------\n", bpf_log_buf);
+
+ if (prog_fd < 0) {
+ printf("Failed to load prog: '%s'\n", strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ ret = bpf_prog_detach(cg_fd, type);
+ printf("bpf_prog_detach() returned '%s' (%d)\n", strerror(errno), errno);
+
+ ret = bpf_prog_attach(prog_fd, cg_fd, type);
+ if (ret < 0) {
+ printf("Failed to attach prog to cgroup: '%s'\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ while (1) {
+ key = MAP_KEY_PACKETS;
+ assert(bpf_lookup_elem(map_fd, &key, &pkt_cnt) == 0);
+
+ key = MAP_KEY_BYTES;
+ assert(bpf_lookup_elem(map_fd, &key, &byte_cnt) == 0);
+
+ printf("cgroup received %lld packets, %lld bytes\n",
+ pkt_cnt, byte_cnt);
+ sleep(1);
+ }
+
+ return EXIT_SUCCESS;
+}
--
2.7.4
^ permalink raw reply related
* [PATCH v7 5/6] net: ipv4, ipv6: run cgroup eBPF egress programs
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun-b10kYP2dOMg, daniel-FeC+5ew28dpmcu3hnIyYJQ,
ast-b10kYP2dOMg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
If the cgroup associated with the receiving socket has an eBPF
programs installed, run them from ip_output(), ip6_output() and
ip_mc_output().
eBPF programs used in this context are expected to either return 1 to
let the packet pass, or != 1 to drop them. The programs have access to
the skb through bpf_skb_load_bytes(), and the payload starts at the
network headers (L3).
Note that cgroup_bpf_run_filter() is stubbed out as static inline nop
for !CONFIG_CGROUP_BPF, and is otherwise guarded by a static key if
the feature is unused.
Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Acked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
net/ipv4/ip_output.c | 17 +++++++++++++++++
net/ipv6/ip6_output.c | 9 +++++++++
2 files changed, 26 insertions(+)
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 05d1058..ee4b249 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -74,6 +74,7 @@
#include <net/checksum.h>
#include <net/inetpeer.h>
#include <net/lwtunnel.h>
+#include <linux/bpf-cgroup.h>
#include <linux/igmp.h>
#include <linux/netfilter_ipv4.h>
#include <linux/netfilter_bridge.h>
@@ -303,6 +304,7 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
struct rtable *rt = skb_rtable(skb);
struct net_device *dev = rt->dst.dev;
+ int ret;
/*
* If the indicated interface is up and running, send the packet.
@@ -312,6 +314,13 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
skb->dev = dev;
skb->protocol = htons(ETH_P_IP);
+ ret = cgroup_bpf_run_filter(sk_to_full_sk(sk), skb,
+ BPF_CGROUP_INET_EGRESS);
+ if (ret) {
+ kfree_skb(skb);
+ return ret;
+ }
+
/*
* Multicasts are looped back for other local users
*/
@@ -364,12 +373,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
int ip_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
struct net_device *dev = skb_dst(skb)->dev;
+ int ret;
IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
skb->dev = dev;
skb->protocol = htons(ETH_P_IP);
+ ret = cgroup_bpf_run_filter(sk_to_full_sk(sk), skb,
+ BPF_CGROUP_INET_EGRESS);
+ if (ret) {
+ kfree_skb(skb);
+ return ret;
+ }
+
return NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING,
net, sk, skb, NULL, dev,
ip_finish_output,
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 6001e78..1947026 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -39,6 +39,7 @@
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/bpf-cgroup.h>
#include <linux/netfilter.h>
#include <linux/netfilter_ipv6.h>
@@ -143,6 +144,7 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
struct net_device *dev = skb_dst(skb)->dev;
struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
+ int ret;
if (unlikely(idev->cnf.disable_ipv6)) {
IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTDISCARDS);
@@ -150,6 +152,13 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
return 0;
}
+ ret = cgroup_bpf_run_filter(sk_to_full_sk(sk), skb,
+ BPF_CGROUP_INET_EGRESS);
+ if (ret) {
+ kfree_skb(skb);
+ return ret;
+ }
+
return NF_HOOK_COND(NFPROTO_IPV6, NF_INET_POST_ROUTING,
net, sk, skb, NULL, dev,
ip6_finish_output,
--
2.7.4
^ permalink raw reply related
* [PATCH v7 3/6] bpf: add BPF_PROG_ATTACH and BPF_PROG_DETACH commands
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun-b10kYP2dOMg, daniel-FeC+5ew28dpmcu3hnIyYJQ,
ast-b10kYP2dOMg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Extend the bpf(2) syscall by two new commands, BPF_PROG_ATTACH and
BPF_PROG_DETACH which allow attaching and detaching eBPF programs
to a target.
On the API level, the target could be anything that has an fd in
userspace, hence the name of the field in union bpf_attr is called
'target_fd'.
When called with BPF_ATTACH_TYPE_CGROUP_INET_{E,IN}GRESS, the target is
expected to be a valid file descriptor of a cgroup v2 directory which
has the bpf controller enabled. These are the only use-cases
implemented by this patch at this point, but more can be added.
If a program of the given type already exists in the given cgroup,
the program is swapped automically, so userspace does not have to drop
an existing program first before installing a new one, which would
otherwise leave a gap in which no program is attached.
For more information on the propagation logic to subcgroups, please
refer to the bpf cgroup controller implementation.
The API is guarded by CAP_NET_ADMIN.
Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Acked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
include/uapi/linux/bpf.h | 8 +++++
kernel/bpf/syscall.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 1f3e6f1..f31b655 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -73,6 +73,8 @@ enum bpf_cmd {
BPF_PROG_LOAD,
BPF_OBJ_PIN,
BPF_OBJ_GET,
+ BPF_PROG_ATTACH,
+ BPF_PROG_DETACH,
};
enum bpf_map_type {
@@ -150,6 +152,12 @@ union bpf_attr {
__aligned_u64 pathname;
__u32 bpf_fd;
};
+
+ struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */
+ __u32 target_fd; /* container object to attach to */
+ __u32 attach_bpf_fd; /* eBPF program to attach */
+ __u32 attach_type;
+ };
} __attribute__((aligned(8)));
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 228f962..1814c01 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -822,6 +822,77 @@ static int bpf_obj_get(const union bpf_attr *attr)
return bpf_obj_get_user(u64_to_ptr(attr->pathname));
}
+#ifdef CONFIG_CGROUP_BPF
+
+#define BPF_PROG_ATTACH_LAST_FIELD attach_type
+
+static int bpf_prog_attach(const union bpf_attr *attr)
+{
+ struct bpf_prog *prog;
+ struct cgroup *cgrp;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (CHECK_ATTR(BPF_PROG_ATTACH))
+ return -EINVAL;
+
+ switch (attr->attach_type) {
+ case BPF_CGROUP_INET_INGRESS:
+ case BPF_CGROUP_INET_EGRESS:
+ prog = bpf_prog_get_type(attr->attach_bpf_fd,
+ BPF_PROG_TYPE_CGROUP_SKB);
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
+
+ cgrp = cgroup_get_from_fd(attr->target_fd);
+ if (IS_ERR(cgrp)) {
+ bpf_prog_put(prog);
+ return PTR_ERR(cgrp);
+ }
+
+ cgroup_bpf_update(cgrp, prog, attr->attach_type);
+ cgroup_put(cgrp);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+#define BPF_PROG_DETACH_LAST_FIELD attach_type
+
+static int bpf_prog_detach(const union bpf_attr *attr)
+{
+ struct cgroup *cgrp;
+
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ if (CHECK_ATTR(BPF_PROG_DETACH))
+ return -EINVAL;
+
+ switch (attr->attach_type) {
+ case BPF_CGROUP_INET_INGRESS:
+ case BPF_CGROUP_INET_EGRESS:
+ cgrp = cgroup_get_from_fd(attr->target_fd);
+ if (IS_ERR(cgrp))
+ return PTR_ERR(cgrp);
+
+ cgroup_bpf_update(cgrp, NULL, attr->attach_type);
+ cgroup_put(cgrp);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_CGROUP_BPF */
+
SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, size)
{
union bpf_attr attr = {};
@@ -888,6 +959,16 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
case BPF_OBJ_GET:
err = bpf_obj_get(&attr);
break;
+
+#ifdef CONFIG_CGROUP_BPF
+ case BPF_PROG_ATTACH:
+ err = bpf_prog_attach(&attr);
+ break;
+ case BPF_PROG_DETACH:
+ err = bpf_prog_detach(&attr);
+ break;
+#endif
+
default:
err = -EINVAL;
break;
--
2.7.4
^ permalink raw reply related
* [PATCH v7 2/6] cgroup: add support for eBPF programs
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun-b10kYP2dOMg, daniel-FeC+5ew28dpmcu3hnIyYJQ,
ast-b10kYP2dOMg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Mack
In-Reply-To: <1477390454-12553-1-git-send-email-daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
This patch adds two sets of eBPF program pointers to struct cgroup.
One for such that are directly pinned to a cgroup, and one for such
that are effective for it.
To illustrate the logic behind that, assume the following example
cgroup hierarchy.
A - B - C
\ D - E
If only B has a program attached, it will be effective for B, C, D
and E. If D then attaches a program itself, that will be effective for
both D and E, and the program in B will only affect B and C. Only one
program of a given type is effective for a cgroup.
Attaching and detaching programs will be done through the bpf(2)
syscall. For now, ingress and egress inet socket filtering are the
only supported use-cases.
Signed-off-by: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Acked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
include/linux/bpf-cgroup.h | 71 +++++++++++++++++++
include/linux/cgroup-defs.h | 4 ++
init/Kconfig | 12 ++++
kernel/bpf/Makefile | 1 +
kernel/bpf/cgroup.c | 167 ++++++++++++++++++++++++++++++++++++++++++++
kernel/cgroup.c | 18 +++++
6 files changed, 273 insertions(+)
create mode 100644 include/linux/bpf-cgroup.h
create mode 100644 kernel/bpf/cgroup.c
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
new file mode 100644
index 0000000..fc076de
--- /dev/null
+++ b/include/linux/bpf-cgroup.h
@@ -0,0 +1,71 @@
+#ifndef _BPF_CGROUP_H
+#define _BPF_CGROUP_H
+
+#include <linux/bpf.h>
+#include <linux/jump_label.h>
+#include <uapi/linux/bpf.h>
+
+struct sock;
+struct cgroup;
+struct sk_buff;
+
+#ifdef CONFIG_CGROUP_BPF
+
+extern struct static_key_false cgroup_bpf_enabled_key;
+#define cgroup_bpf_enabled static_branch_unlikely(&cgroup_bpf_enabled_key)
+
+struct cgroup_bpf {
+ /*
+ * Store two sets of bpf_prog pointers, one for programs that are
+ * pinned directly to this cgroup, and one for those that are effective
+ * when this cgroup is accessed.
+ */
+ struct bpf_prog *prog[MAX_BPF_ATTACH_TYPE];
+ struct bpf_prog *effective[MAX_BPF_ATTACH_TYPE];
+};
+
+void cgroup_bpf_put(struct cgroup *cgrp);
+void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent);
+
+void __cgroup_bpf_update(struct cgroup *cgrp,
+ struct cgroup *parent,
+ struct bpf_prog *prog,
+ enum bpf_attach_type type);
+
+/* Wrapper for __cgroup_bpf_update() protected by cgroup_mutex */
+void cgroup_bpf_update(struct cgroup *cgrp,
+ struct bpf_prog *prog,
+ enum bpf_attach_type type);
+
+int __cgroup_bpf_run_filter(struct sock *sk,
+ struct sk_buff *skb,
+ enum bpf_attach_type type);
+
+/* Wrapper for __cgroup_bpf_run_filter() guarded by cgroup_bpf_enabled */
+static inline int cgroup_bpf_run_filter(struct sock *sk,
+ struct sk_buff *skb,
+ enum bpf_attach_type type)
+{
+ if (cgroup_bpf_enabled)
+ return __cgroup_bpf_run_filter(sk, skb, type);
+
+ return 0;
+}
+
+#else
+
+struct cgroup_bpf {};
+static inline void cgroup_bpf_put(struct cgroup *cgrp) {}
+static inline void cgroup_bpf_inherit(struct cgroup *cgrp,
+ struct cgroup *parent) {}
+
+static inline int cgroup_bpf_run_filter(struct sock *sk,
+ struct sk_buff *skb,
+ enum bpf_attach_type type)
+{
+ return 0;
+}
+
+#endif /* CONFIG_CGROUP_BPF */
+
+#endif /* _BPF_CGROUP_H */
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5b17de6..861b467 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -16,6 +16,7 @@
#include <linux/percpu-refcount.h>
#include <linux/percpu-rwsem.h>
#include <linux/workqueue.h>
+#include <linux/bpf-cgroup.h>
#ifdef CONFIG_CGROUPS
@@ -300,6 +301,9 @@ struct cgroup {
/* used to schedule release agent */
struct work_struct release_agent_work;
+ /* used to store eBPF programs */
+ struct cgroup_bpf bpf;
+
/* ids of the ancestors at each level including self */
int ancestor_ids[];
};
diff --git a/init/Kconfig b/init/Kconfig
index 34407f1..405120b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1154,6 +1154,18 @@ config CGROUP_PERF
Say N if unsure.
+config CGROUP_BPF
+ bool "Support for eBPF programs attached to cgroups"
+ depends on BPF_SYSCALL && SOCK_CGROUP_DATA
+ help
+ Allow attaching eBPF programs to a cgroup using the bpf(2)
+ syscall command BPF_PROG_ATTACH.
+
+ In which context these programs are accessed depends on the type
+ of attachment. For instance, programs that are attached using
+ BPF_CGROUP_INET_INGRESS will be executed on the ingress path of
+ inet sockets.
+
config CGROUP_DEBUG
bool "Example controller"
default n
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index eed911d..b22256b 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o
ifeq ($(CONFIG_PERF_EVENTS),y)
obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
endif
+obj-$(CONFIG_CGROUP_BPF) += cgroup.o
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
new file mode 100644
index 0000000..a0ab43f
--- /dev/null
+++ b/kernel/bpf/cgroup.c
@@ -0,0 +1,167 @@
+/*
+ * Functions to manage eBPF programs attached to cgroups
+ *
+ * Copyright (c) 2016 Daniel Mack
+ *
+ * This file is subject to the terms and conditions of version 2 of the GNU
+ * General Public License. See the file COPYING in the main directory of the
+ * Linux distribution for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/atomic.h>
+#include <linux/cgroup.h>
+#include <linux/slab.h>
+#include <linux/bpf.h>
+#include <linux/bpf-cgroup.h>
+#include <net/sock.h>
+
+DEFINE_STATIC_KEY_FALSE(cgroup_bpf_enabled_key);
+EXPORT_SYMBOL(cgroup_bpf_enabled_key);
+
+/**
+ * cgroup_bpf_put() - put references of all bpf programs
+ * @cgrp: the cgroup to modify
+ */
+void cgroup_bpf_put(struct cgroup *cgrp)
+{
+ unsigned int type;
+
+ for (type = 0; type < ARRAY_SIZE(cgrp->bpf.prog); type++) {
+ struct bpf_prog *prog = cgrp->bpf.prog[type];
+
+ if (prog) {
+ bpf_prog_put(prog);
+ static_branch_dec(&cgroup_bpf_enabled_key);
+ }
+ }
+}
+
+/**
+ * cgroup_bpf_inherit() - inherit effective programs from parent
+ * @cgrp: the cgroup to modify
+ * @parent: the parent to inherit from
+ */
+void cgroup_bpf_inherit(struct cgroup *cgrp, struct cgroup *parent)
+{
+ unsigned int type;
+
+ for (type = 0; type < ARRAY_SIZE(cgrp->bpf.effective); type++) {
+ struct bpf_prog *e;
+
+ e = rcu_dereference_protected(parent->bpf.effective[type],
+ lockdep_is_held(&cgroup_mutex));
+ rcu_assign_pointer(cgrp->bpf.effective[type], e);
+ }
+}
+
+/**
+ * __cgroup_bpf_update() - Update the pinned program of a cgroup, and
+ * propagate the change to descendants
+ * @cgrp: The cgroup which descendants to traverse
+ * @parent: The parent of @cgrp, or %NULL if @cgrp is the root
+ * @prog: A new program to pin
+ * @type: Type of pinning operation (ingress/egress)
+ *
+ * Each cgroup has a set of two pointers for bpf programs; one for eBPF
+ * programs it owns, and which is effective for execution.
+ *
+ * If @prog is %NULL, this function attaches a new program to the cgroup and
+ * releases the one that is currently attached, if any. @prog is then made
+ * the effective program of type @type in that cgroup.
+ *
+ * If @prog is %NULL, the currently attached program of type @type is released,
+ * and the effective program of the parent cgroup (if any) is inherited to
+ * @cgrp.
+ *
+ * Then, the descendants of @cgrp are walked and the effective program for
+ * each of them is set to the effective program of @cgrp unless the
+ * descendant has its own program attached, in which case the subbranch is
+ * skipped. This ensures that delegated subcgroups with own programs are left
+ * untouched.
+ *
+ * Must be called with cgroup_mutex held.
+ */
+void __cgroup_bpf_update(struct cgroup *cgrp,
+ struct cgroup *parent,
+ struct bpf_prog *prog,
+ enum bpf_attach_type type)
+{
+ struct bpf_prog *old_prog, *effective;
+ struct cgroup_subsys_state *pos;
+
+ old_prog = xchg(cgrp->bpf.prog + type, prog);
+
+ effective = (!prog && parent) ?
+ rcu_dereference_protected(parent->bpf.effective[type],
+ lockdep_is_held(&cgroup_mutex)) :
+ prog;
+
+ css_for_each_descendant_pre(pos, &cgrp->self) {
+ struct cgroup *desc = container_of(pos, struct cgroup, self);
+
+ /* skip the subtree if the descendant has its own program */
+ if (desc->bpf.prog[type] && desc != cgrp)
+ pos = css_rightmost_descendant(pos);
+ else
+ rcu_assign_pointer(desc->bpf.effective[type],
+ effective);
+ }
+
+ if (prog)
+ static_branch_inc(&cgroup_bpf_enabled_key);
+
+ if (old_prog) {
+ bpf_prog_put(old_prog);
+ static_branch_dec(&cgroup_bpf_enabled_key);
+ }
+}
+
+/**
+ * __cgroup_bpf_run_filter() - Run a program for packet filtering
+ * @sk: The socken sending or receiving traffic
+ * @skb: The skb that is being sent or received
+ * @type: The type of program to be exectuted
+ *
+ * If no socket is passed, or the socket is not of type INET or INET6,
+ * this function does nothing and returns 0.
+ *
+ * The program type passed in via @type must be suitable for network
+ * filtering. No further check is performed to assert that.
+ *
+ * This function will return %-EPERM if any if an attached program was found
+ * and if it returned != 1 during execution. In all other cases, 0 is returned.
+ */
+int __cgroup_bpf_run_filter(struct sock *sk,
+ struct sk_buff *skb,
+ enum bpf_attach_type type)
+{
+ struct bpf_prog *prog;
+ struct cgroup *cgrp;
+ int ret = 0;
+
+ if (!sk || !sk_fullsock(sk))
+ return 0;
+
+ if (sk->sk_family != AF_INET &&
+ sk->sk_family != AF_INET6)
+ return 0;
+
+ cgrp = sock_cgroup_ptr(&sk->sk_cgrp_data);
+
+ rcu_read_lock();
+
+ prog = rcu_dereference(cgrp->bpf.effective[type]);
+ if (prog) {
+ unsigned int offset = skb->data - skb_network_header(skb);
+
+ __skb_push(skb, offset);
+ ret = bpf_prog_run_save_cb(prog, skb) == 1 ? 0 : -EPERM;
+ __skb_pull(skb, offset);
+ }
+
+ rcu_read_unlock();
+
+ return ret;
+}
+EXPORT_SYMBOL(__cgroup_bpf_run_filter);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 85bc9be..2ee9ec3 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -5074,6 +5074,8 @@ static void css_release_work_fn(struct work_struct *work)
if (cgrp->kn)
RCU_INIT_POINTER(*(void __rcu __force **)&cgrp->kn->priv,
NULL);
+
+ cgroup_bpf_put(cgrp);
}
mutex_unlock(&cgroup_mutex);
@@ -5281,6 +5283,9 @@ static struct cgroup *cgroup_create(struct cgroup *parent)
if (!cgroup_on_dfl(cgrp))
cgrp->subtree_control = cgroup_control(cgrp);
+ if (parent)
+ cgroup_bpf_inherit(cgrp, parent);
+
cgroup_propagate_control(cgrp);
/* @cgrp doesn't have dir yet so the following will only create csses */
@@ -6495,6 +6500,19 @@ static __init int cgroup_namespaces_init(void)
}
subsys_initcall(cgroup_namespaces_init);
+#ifdef CONFIG_CGROUP_BPF
+void cgroup_bpf_update(struct cgroup *cgrp,
+ struct bpf_prog *prog,
+ enum bpf_attach_type type)
+{
+ struct cgroup *parent = cgroup_parent(cgrp);
+
+ mutex_lock(&cgroup_mutex);
+ __cgroup_bpf_update(cgrp, parent, prog, type);
+ mutex_unlock(&cgroup_mutex);
+}
+#endif /* CONFIG_CGROUP_BPF */
+
#ifdef CONFIG_CGROUP_DEBUG
static struct cgroup_subsys_state *
debug_css_alloc(struct cgroup_subsys_state *parent_css)
--
2.7.4
^ permalink raw reply related
* [PATCH v7 0/6] Add eBPF hooks for cgroups
From: Daniel Mack @ 2016-10-25 10:14 UTC (permalink / raw)
To: htejun-b10kYP2dOMg, daniel-FeC+5ew28dpmcu3hnIyYJQ,
ast-b10kYP2dOMg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Mack
This is v7 of the patch set to allow eBPF programs for network
filtering and accounting to be attached to cgroups, so that they apply
to all sockets of all tasks placed in that cgroup. The logic also
allows to be extendeded for other cgroup based eBPF logic.
Although there are some minor updates as listed below, the overall
concept remains the same. Alexei, Daniel Borkmann and I have been
discussing some details off-list for some time which I'd like to
summarize quickly.
* Regarding introspection of installed eBPF programs, possible
solutions are a) to add trace points to the bpf(2) syscall to dump
programs when they are installed (which could be done by some
sort of daemon), b) to use audit logging.
Dumping programs once they are installed is problematic because of
the internal optimizations done to the eBPF program during its
lifetime. Also, the references to maps etc. would need to be
restored during the dump.
Just exposing whether or not a program is attached would be
trivial to do, however, most easily through another bpf(2)
command. That can be added later on though.
* Moving the filter hooks to some other sub-system such as tc or
xdp is problematic because they are centered around net devices,
which the current implementation is agnostic of, for good reasons.
Also, we're back to square one then, because the local receiver,
along with its cgroup membership, is not guaranteed to be known
at that point in time; and that is the problem class what brought
us here in the first place.
So, here we go again. Thanks for having another look.
Thanks,
Daniel
Changes from v6:
* Rebased to 4.9-rc2
* Add EXPORT_SYMBOL(__cgroup_bpf_run_filter). The kbuild test robot
now succeeds in building this version of the patch set.
* Switch from bpf_prog_run_save_cb() to bpf_prog_run_clear_cb() to not
tamper with the contents of skb->cb[]. Pointed out by Daniel
Borkmann.
* Use sk_to_full_sk() in the egress path, as suggested by Daniel
Borkmann.
* Renamed BPF_PROG_TYPE_CGROUP_SOCKET to BPF_PROG_TYPE_CGROUP_SKB, as
requested by David Ahern.
* Added Alexei's Acked-by tags.
Changes from v5:
* The eBPF programs now operate on L3 rather than on L2 of the packets,
and the egress hooks were moved from __dev_queue_xmit() to
ip*_output().
* For BPF_PROG_TYPE_CGROUP_SOCKET, disallow direct access to the skb
through BPF_LD_[ABS|IND] instructions, but hook up the
bpf_skb_load_bytes() access helper instead. Thanks to Daniel Borkmann
for the help.
Changes from v4:
* Plug an skb leak when dropping packets due to eBPF verdicts in
__dev_queue_xmit(). Spotted by Daniel Borkmann.
* Check for sk_fullsock(sk) in __cgroup_bpf_run_filter() so we don't
operate on timewait or request sockets. Suggested by Daniel Borkmann.
* Add missing @parent parameter in kerneldoc of __cgroup_bpf_update().
Spotted by Rami Rosen.
* Include linux/jump_label.h from bpf-cgroup.h to fix a kbuild error.
Changes from v3:
* Dropped the _FILTER suffix from BPF_PROG_TYPE_CGROUP_SOCKET_FILTER,
renamed BPF_ATTACH_TYPE_CGROUP_INET_{E,IN}GRESS to
BPF_CGROUP_INET_{IN,E}GRESS and alias BPF_MAX_ATTACH_TYPE to
__BPF_MAX_ATTACH_TYPE, as suggested by Daniel Borkmann.
* Dropped the attach_flags member from the anonymous struct for BPF
attach operations in union bpf_attr. They can be added later on via
CHECK_ATTR. Requested by Daniel Borkmann and Alexei.
* Release old_prog at the end of __cgroup_bpf_update rather that at
the beginning to fix a race gap between program updates and their
users. Spotted by Daniel Borkmann.
* Plugged an skb leak when dropping packets on the egress path.
Spotted by Daniel Borkmann.
* Add cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org to the loop, as suggested by Rami Rosen.
* Some minor coding style adoptions not worth mentioning in particular.
Changes from v2:
* Fixed the RCU locking details Tejun pointed out.
* Assert bpf_attr.flags == 0 in BPF_PROG_DETACH syscall handler.
Changes from v1:
* Moved all bpf specific cgroup code into its own file, and stub
out related functions for !CONFIG_CGROUP_BPF as static inline nops.
This way, the call sites are not cluttered with #ifdef guards while
the feature remains compile-time configurable.
* Implemented the new scheme proposed by Tejun. Per cgroup, store one
set of pointers that are pinned to the cgroup, and one for the
programs that are effective. When a program is attached or detached,
the change is propagated to all the cgroup's descendants. If a
subcgroup has its own pinned program, skip the whole subbranch in
order to allow delegation models.
* The hookup for egress packets is now done from __dev_queue_xmit().
* A static key is now used in both the ingress and egress fast paths
to keep performance penalties close to zero if the feature is
not in use.
* Overall cleanup to make the accessors use the program arrays.
This should make it much easier to add new program types, which
will then automatically follow the pinned vs. effective logic.
* Fixed locking issues, as pointed out by Eric Dumazet and Alexei
Starovoitov. Changes to the program array are now done with
xchg() and are protected by cgroup_mutex.
* eBPF programs are now expected to return 1 to let the packet pass,
not >= 0. Pointed out by Alexei.
* Operation is now limited to INET sockets, so local AF_UNIX sockets
are not affected. The enum members are renamed accordingly. In case
other socket families should be supported, this can be extended in
the future.
* The sample program learned to support both ingress and egress, and
can now optionally make the eBPF program drop packets by making it
return 0.
Daniel Mack (6):
bpf: add new prog type for cgroup socket filtering
cgroup: add support for eBPF programs
bpf: add BPF_PROG_ATTACH and BPF_PROG_DETACH commands
net: filter: run cgroup eBPF ingress programs
net: ipv4, ipv6: run cgroup eBPF egress programs
samples: bpf: add userspace example for attaching eBPF programs to
cgroups
include/linux/bpf-cgroup.h | 71 +++++++++++++++++
include/linux/cgroup-defs.h | 4 +
include/uapi/linux/bpf.h | 17 ++++
init/Kconfig | 12 +++
kernel/bpf/Makefile | 1 +
kernel/bpf/cgroup.c | 167 ++++++++++++++++++++++++++++++++++++++++
kernel/bpf/syscall.c | 81 +++++++++++++++++++
kernel/cgroup.c | 18 +++++
net/core/filter.c | 27 +++++++
net/ipv4/ip_output.c | 17 ++++
net/ipv6/ip6_output.c | 9 +++
samples/bpf/Makefile | 2 +
samples/bpf/libbpf.c | 21 +++++
samples/bpf/libbpf.h | 3 +
samples/bpf/test_cgrp2_attach.c | 147 +++++++++++++++++++++++++++++++++++
15 files changed, 597 insertions(+)
create mode 100644 include/linux/bpf-cgroup.h
create mode 100644 kernel/bpf/cgroup.c
create mode 100644 samples/bpf/test_cgrp2_attach.c
--
2.7.4
^ permalink raw reply
* [patch net 1/2] mlxsw: spectrum_router: Save requested prefix bitlist when creating tree
From: Jiri Pirko @ 2016-10-25 9:25 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, eladr, yotamg, nogahf, ogerlitz
In-Reply-To: <1477387557-5048-1-git-send-email-jiri@resnulli.us>
From: Jiri Pirko <jiri@mellanox.com>
Currently, the prefix bitlist is not saved for LPM trees, causing the
compare to always fail which causes the tree to be destroyed and created
for every inserted and removed FIB entry. So fix this by saving
the bitlist as it should have been done from the very beginning.
Fixes: 53342023eed9 ("mlxsw: spectrum_router: Implement LPM trees management")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index f3d50d3..4cff4c0 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -320,6 +320,8 @@ mlxsw_sp_lpm_tree_create(struct mlxsw_sp *mlxsw_sp,
lpm_tree);
if (err)
goto err_left_struct_set;
+ memcpy(&lpm_tree->prefix_usage, prefix_usage,
+ sizeof(lpm_tree->prefix_usage));
return lpm_tree;
err_left_struct_set:
--
2.5.5
^ permalink raw reply related
* [patch net 2/2] mlxsw: spectrum_router: Compare only trees which are in use during tree get
From: Jiri Pirko @ 2016-10-25 9:25 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, eladr, yotamg, nogahf, ogerlitz
In-Reply-To: <1477387557-5048-1-git-send-email-jiri@resnulli.us>
From: Jiri Pirko <jiri@mellanox.com>
Only trees which are in use should be compared to requested prefix usage.
Fixes: 53342023eed9 ("mlxsw: spectrum_router: Implement LPM trees management")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 4cff4c0..4573da2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -345,7 +345,8 @@ mlxsw_sp_lpm_tree_get(struct mlxsw_sp *mlxsw_sp,
for (i = 0; i < MLXSW_SP_LPM_TREE_COUNT; i++) {
lpm_tree = &mlxsw_sp->router.lpm_trees[i];
- if (lpm_tree->proto == proto &&
+ if (lpm_tree->ref_count != 0 &&
+ lpm_tree->proto == proto &&
mlxsw_sp_prefix_usage_eq(&lpm_tree->prefix_usage,
prefix_usage))
goto inc_ref_count;
--
2.5.5
^ permalink raw reply related
* [patch net 0/2] mlxsw: Couple of fixes
From: Jiri Pirko @ 2016-10-25 9:25 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, eladr, yotamg, nogahf, ogerlitz
From: Jiri Pirko <jiri@mellanox.com>
Couple of LPM tree management fixes.
Jiri Pirko (2):
mlxsw: spectrum_router: Save requested prefix bitlist when creating
tree
mlxsw: spectrum_router: Compare only trees which are in use during
tree get
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--
2.5.5
^ permalink raw reply
* Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit
From: Xin Long @ 2016-10-25 9:05 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev@vger.kernel.org, Marcelo Ricardo Leitner, Vlad Yasevich,
Daniel Borkmann, David Miller, linux-sctp@vger.kernel.org,
Michael Tuexen, Eric Dumazet, Brenda Butler, gabor
In-Reply-To: <369b7b2c-d1fb-5dd2-30b9-4f54400aa770@mojatatu.com>
>> in case [1], user can't see the ENOMEM, ENOMEM is more like
>> a internal err.
>>
>
> Still not clear. Are you saying, say an old kernel like 3.11 would
> not return the user ENOMEN for the use case[1] you fixed? I am not
> talking post your fix.
Sorry for confusing you.
3.11 would return the user ENOMEN for the use case[1].
but this behavior is incorrect, it's not consistent with tcp.
>
>> in case [2], user will got the ENOMEM, they should resend this msg,
>> It's the the general case mentioned-above
>>
>
> I am trying to see if we can avoid backporting this fix to 3.11.
> In [1], is ENOMEM propagated to user space (dont talk about your
> fix, I mean pre-your-fix).
yes, in [1], pre-my-fix, ENOMEM is propagated to user space.
>
>
>> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
>> also may fail to alloc skb, but it doesn't return any err to user, just
>> like
>> sctp_packet_transmit. That's why I don't think we should change something
>> in manpage, as here sctp is consistent with tcp now.
>>
>> make sense ?
>
>
> No ;-> The manpage is bad. Go look at it. In the case of ENOBUFS or
> EMSGSIZE it is clear what needs to be done.
> If the answer is _on ENOMEM_ user must resend then thats what we need
> to say.
yes, on ENOMEM user must resend if he want send out this msg successfully.
^ permalink raw reply
* Re: [PATCH (net.git)] net: phy: at803x: disable by default the hibernation feature
From: Andrew Lunn @ 2016-10-25 9:00 UTC (permalink / raw)
To: Giuseppe Cavallaro; +Cc: netdev, Matus Ujhelyi
In-Reply-To: <1477384282-17878-1-git-send-email-peppe.cavallaro@st.com>
> For example, while booting a Kernel the SYNP MAC (stmmac) fails
> to initialize own DMA engine if the phy entered in hibernation
> before.
Have you tried fixing stmmac instead?
Andrew
^ permalink raw reply
* RE: [PATCH v2 RESEND] xen-netback: prefer xenbus_scanf() over xenbus_gather()
From: Paul Durrant @ 2016-10-25 8:52 UTC (permalink / raw)
To: Jan Beulich
Cc: David Vrabel, Wei Liu, xen-devel@lists.xenproject.org,
boris.ostrovsky@oracle.com, Juergen Gross, netdev@vger.kernel.org
In-Reply-To: <580F32670200007800119584@prv-mh.provo.novell.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 October 2016 09:23
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> xen-devel@lists.xenproject.org; boris.ostrovsky@oracle.com; Juergen Gross
> <JGross@suse.com>; netdev@vger.kernel.org
> Subject: RE: [PATCH v2 RESEND] xen-netback: prefer xenbus_scanf() over
> xenbus_gather()
>
> >>> On 25.10.16 at 09:52, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 24 October 2016 16:08
> >> --- 4.9-rc2/drivers/net/xen-netback/xenbus.c
> >> +++ 4.9-rc2-xen-netback-prefer-xenbus_scanf/drivers/net/xen-
> netback/xenbus.c
> >> @@ -889,16 +889,16 @@ static int connect_ctrl_ring(struct back
> >> unsigned int evtchn;
> >> int err;
> >>
> >> - err = xenbus_gather(XBT_NIL, dev->otherend,
> >> - "ctrl-ring-ref", "%u", &val, NULL);
> >> - if (err)
> >> + err = xenbus_scanf(XBT_NIL, dev->otherend,
> >> + "ctrl-ring-ref", "%u", &val);
> >> + if (err <= 0)
> >
> > Looking at other uses of xenbus_scanf() in the same code I think the check
> > here should be if (err < 0). It's a nit, since xenbus_scanf() cannot return 0,
> > but it would be better for consistency I think.
>
> Hmm, this goes back to the discussion following from
> https://lists.xenproject.org/archives/html/xen-devel/2016-
> 07/msg00678.html
> which in fact you had given your R-b back then. I continue to be
> of the opinion that callers should not leverage the fact that
> xenbus_scanf() can't return zero. They instead should check for
> an explicit success indicator (which only positive values are). But
> you're the maintainer of the code, so if you now think the same
> way David does, I guess I'll have to make the adjustment.
>
> >> goto done; /* The frontend does not have a control ring */
> >>
> >> ring_ref = val;
> >>
> >> - err = xenbus_gather(XBT_NIL, dev->otherend,
> >> - "event-channel-ctrl", "%u", &val, NULL);
> >> - if (err) {
> >> + err = xenbus_scanf(XBT_NIL, dev->otherend,
> >> + "event-channel-ctrl", "%u", &val);
> >> + if (err <= 0) {
> >> xenbus_dev_fatal(dev, err,
> >> "reading %s/event-channel-ctrl",
> >> dev->otherend);
> >> @@ -919,7 +919,7 @@ done:
> >> return 0;
> >>
> >> fail:
> >> - return err;
> >> + return err ?: -ENODATA;
> >
> > I don't think you need this.
>
> If the other change gets made, then indeed this isn't needed.
Yes, and that's why I prefer to opt for consistency with other code in this case.
Paul
>
> Jan
^ permalink raw reply
* [PATCH (net.git)] net: phy: at803x: disable by default the hibernation feature
From: Giuseppe Cavallaro @ 2016-10-25 8:31 UTC (permalink / raw)
To: netdev; +Cc: Giuseppe Cavallaro, Matus Ujhelyi
These PHY chips, by default, enable the hibernation feature
so, if the cable is unplugged the device enters in hibernation
mode after some time. This can generate problems on some cases.
It has been noticed, on some platforms that, if the phy enters
in hibernation, the missing of the rx clock signal can force
a mac to fail when setup some parts that need to be properly
clocked.
For example, while booting a Kernel the SYNP MAC (stmmac) fails
to initialize own DMA engine if the phy entered in hibernation
before.
So, the patch just disables this feature by default when init
the PHY driver.
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Matus Ujhelyi <ujhelyi.m@gmail.com>
---
drivers/net/phy/at803x.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f279a89..d60953e 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -52,6 +52,9 @@
#define AT803X_DEBUG_REG_5 0x05
#define AT803X_DEBUG_TX_CLK_DLY_EN BIT(8)
+#define AT803X_DEBUG_REG_B 0x0B
+#define AT803X_DEBUG_PS_HIB_EN BIT(15)
+
#define AT803X_REG_CHIP_CONFIG 0x1f
#define AT803X_BT_BX_REG_SEL 0x8000
@@ -117,6 +120,12 @@ static inline int at803x_enable_tx_delay(struct phy_device *phydev)
AT803X_DEBUG_TX_CLK_DLY_EN);
}
+static inline int at803x_disable_hibernation(struct phy_device *phydev)
+{
+ return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_B,
+ AT803X_DEBUG_PS_HIB_EN, 0);
+}
+
/* save relevant PHY registers to private copy */
static void at803x_context_save(struct phy_device *phydev,
struct at803x_context *context)
@@ -314,6 +323,9 @@ static int at803x_config_init(struct phy_device *phydev)
return ret;
}
+ /* By default disable the Power Hibernation feature */
+ at803x_disable_hibernation(phydev);
+
return 0;
}
--
2.7.4
^ permalink raw reply related
* RE: [PATCH v2 RESEND] xen-netback: prefer xenbus_scanf() over xenbus_gather()
From: Jan Beulich @ 2016-10-25 8:22 UTC (permalink / raw)
To: Paul Durrant
Cc: David Vrabel, Wei Liu, xen-devel@lists.xenproject.org,
boris.ostrovsky@oracle.com, Juergen Gross, netdev@vger.kernel.org
In-Reply-To: <8cc49b57d5d64a439ddc99b94c99f9a3@AMSPEX02CL03.citrite.net>
>>> On 25.10.16 at 09:52, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 24 October 2016 16:08
>> --- 4.9-rc2/drivers/net/xen-netback/xenbus.c
>> +++ 4.9-rc2-xen-netback-prefer-xenbus_scanf/drivers/net/xen-netback/xenbus.c
>> @@ -889,16 +889,16 @@ static int connect_ctrl_ring(struct back
>> unsigned int evtchn;
>> int err;
>>
>> - err = xenbus_gather(XBT_NIL, dev->otherend,
>> - "ctrl-ring-ref", "%u", &val, NULL);
>> - if (err)
>> + err = xenbus_scanf(XBT_NIL, dev->otherend,
>> + "ctrl-ring-ref", "%u", &val);
>> + if (err <= 0)
>
> Looking at other uses of xenbus_scanf() in the same code I think the check
> here should be if (err < 0). It's a nit, since xenbus_scanf() cannot return 0,
> but it would be better for consistency I think.
Hmm, this goes back to the discussion following from
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00678.html
which in fact you had given your R-b back then. I continue to be
of the opinion that callers should not leverage the fact that
xenbus_scanf() can't return zero. They instead should check for
an explicit success indicator (which only positive values are). But
you're the maintainer of the code, so if you now think the same
way David does, I guess I'll have to make the adjustment.
>> goto done; /* The frontend does not have a control ring */
>>
>> ring_ref = val;
>>
>> - err = xenbus_gather(XBT_NIL, dev->otherend,
>> - "event-channel-ctrl", "%u", &val, NULL);
>> - if (err) {
>> + err = xenbus_scanf(XBT_NIL, dev->otherend,
>> + "event-channel-ctrl", "%u", &val);
>> + if (err <= 0) {
>> xenbus_dev_fatal(dev, err,
>> "reading %s/event-channel-ctrl",
>> dev->otherend);
>> @@ -919,7 +919,7 @@ done:
>> return 0;
>>
>> fail:
>> - return err;
>> + return err ?: -ENODATA;
>
> I don't think you need this.
If the other change gets made, then indeed this isn't needed.
Jan
^ 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