* [net-next PATCH] dql: add a burst attribute
From: Eric Dumazet @ 2014-09-30 12:51 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <1412080482.30721.68.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Add a new dql attribute, to control how much packets we are allowed to
burst from qdisc to device.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Jesper, please consider using this instead of 7 or 8 values you hard coded.
include/linux/dynamic_queue_limits.h | 1
lib/dynamic_queue_limits.c | 1
net/core/net-sysfs.c | 31 +++++++++++++++++++++++++
3 files changed, 33 insertions(+)
diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
index a4be70398ce1..8509053eb330 100644
--- a/include/linux/dynamic_queue_limits.h
+++ b/include/linux/dynamic_queue_limits.h
@@ -42,6 +42,7 @@ struct dql {
unsigned int num_queued; /* Total ever queued */
unsigned int adj_limit; /* limit + num_completed */
unsigned int last_obj_cnt; /* Count at last queuing */
+ unsigned int burst;
/* Fields accessed only by completion path (dql_completed) */
diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
index 0777c5a45fa0..acc0d6105191 100644
--- a/lib/dynamic_queue_limits.c
+++ b/lib/dynamic_queue_limits.c
@@ -132,6 +132,7 @@ int dql_init(struct dql *dql, unsigned hold_time)
dql->max_limit = DQL_MAX_LIMIT;
dql->min_limit = 0;
dql->slack_hold_time = hold_time;
+ dql->burst = 7;
dql_reset(dql);
return 0;
}
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 9dd06699b09c..80cf6f166706 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -978,6 +978,36 @@ static struct netdev_queue_attribute bql_hold_time_attribute =
__ATTR(hold_time, S_IRUGO | S_IWUSR, bql_show_hold_time,
bql_set_hold_time);
+static ssize_t bql_show_burst(struct netdev_queue *queue,
+ struct netdev_queue_attribute *attr,
+ char *buf)
+{
+ const struct dql *dql = &queue->dql;
+
+ return sprintf(buf, "%u\n", dql->burst);
+}
+
+static ssize_t bql_set_burst(struct netdev_queue *queue,
+ struct netdev_queue_attribute *attribute,
+ const char *buf, size_t len)
+{
+ struct dql *dql = &queue->dql;
+ unsigned int value;
+ int err;
+
+ err = kstrtouint(buf, 10, &value);
+ if (err < 0)
+ return err;
+
+ dql->burst = value;
+
+ return len;
+}
+
+static struct netdev_queue_attribute bql_burst_attribute =
+ __ATTR(burst, S_IRUGO | S_IWUSR, bql_show_burst,
+ bql_set_burst);
+
static ssize_t bql_show_inflight(struct netdev_queue *queue,
struct netdev_queue_attribute *attr,
char *buf)
@@ -1019,6 +1049,7 @@ static struct attribute *dql_attrs[] = {
&bql_limit_min_attribute.attr,
&bql_hold_time_attribute.attr,
&bql_inflight_attribute.attr,
+ &bql_burst_attribute.attr,
NULL
};
^ permalink raw reply related
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Florian Westphal @ 2014-09-30 12:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: Florian Westphal, Jesper Dangaard Brouer, netdev, David S. Miller,
Tom Herbert, Hannes Frederic Sowa, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
toke
In-Reply-To: <1412080721.30721.72.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> I guess my question was more like : Should I wait you sending patches,
> or should I do this ?
Sure, if you have time to work on this go ahead.
Thanks Eric!
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 12:38 UTC (permalink / raw)
To: Florian Westphal
Cc: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Hannes Frederic Sowa, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930122509.GF11709@breakpoint.cc>
On Tue, 2014-09-30 at 14:25 +0200, Florian Westphal wrote:
> You're right, but this is not related to this patch specifically.
> But sure, we can look into moving validate_xmit_skb calls outside of the
> qdisc root locked sections.
Yes please.
>
> > 2) TSO support. Have you ideas how to perform this ?
>
> Yes, the idea was to just remove the skb_is_gso() test and limit
> dql_avail() result to e.g. 128k to avoid a '8-fat-gso-skbs-in-a-row' scenario.
Really I think you are mistaken about GSO/TSO being fat skb. This is not
a qdisc concern, but packets producer. Think of TSO autosizing which
already takes care of this.
>
> Did you have any other ideas in mind?
I guess my question was more like : Should I wait you sending patches,
or should I do this ?
We are approaching merge window, and I would like to get this sorted out
for linux-3.18
Thanks
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 12:34 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On Tue, 2014-09-30 at 10:53 +0200, Jesper Dangaard Brouer wrote:
> For now, as a conservative approach, don't bulk dequeue GSO and
> segmented GSO packets, because testing showed requeuing occuring
> with segmented GSO packets.
Note that GSO happens after qdisc dequeue.
We should not care if part of a GSO train is stopped because TX ring is
full. If we stop a GSO train, then we only lower GRO aggregation for the
receiver.
Normally, if BQL is properly working, we should practically not hit TX
ring buffer limit, unless you send very small packets just to stress the
thing (synthetic benchmark, not a real workload)
We should not stop a GSO train because of BQL : Remind that BQL is to
avoid head of line blocking, but there is no way another high prio
packet can be sent on the wire before one if the packet resulting of GSO
segments.
So telling that requeueing is happening with GSO is kind of irrelevant.
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Florian Westphal @ 2014-09-30 12:25 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
toke
In-Reply-To: <1412077736.30721.54.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > struct sk_buff *skb = q->gso_skb;
> > @@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> > } else
> > skb = NULL;
> > } else {
> > - if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
> > + if (!(q->flags & TCQ_F_ONETXQUEUE) ||
> > + !netif_xmit_frozen_or_stopped(txq)) {
> > + int bytelimit = qdisc_avail_bulklimit(txq);
> > +
> > skb = q->dequeue(q);
> > - if (skb)
> > + if (skb) {
> > + bytelimit -= skb->len;
> > skb = validate_xmit_skb(skb, qdisc_dev(q));
> > + }
> > + if (skb && qdisc_may_bulk(q))
> > + skb = try_bulk_dequeue_skb(q, skb, bytelimit);
> > }
> > }
>
> Okay...
>
> In my opinion, you guys missed some opportunities :
>
> 1) Ideally the algo should try to call validate_xmit_skb() outside of
> the locks (qdisc lock or device lock), because eventually doing checksum
> computation or full segmentation should allow other cpus doing enqueues.
You're right, but this is not related to this patch specifically.
But sure, we can look into moving validate_xmit_skb calls outside of the
qdisc root locked sections.
> 2) TSO support. Have you ideas how to perform this ?
Yes, the idea was to just remove the skb_is_gso() test and limit
dql_avail() result to e.g. 128k to avoid a '8-fat-gso-skbs-in-a-row' scenario.
Did you have any other ideas in mind?
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v2 net-next] mlx4: optimize xmit path
From: Eric Dumazet @ 2014-09-30 12:11 UTC (permalink / raw)
To: Amir Vadai
Cc: Or Gerlitz, Alexei Starovoitov, David S. Miller,
Jesper Dangaard Brouer, Eric Dumazet, John Fastabend,
Linux Netdev List, Or Gerlitz
In-Reply-To: <542A9BAC.6010208@mellanox.com>
On Tue, 2014-09-30 at 15:01 +0300, Amir Vadai wrote:
> Hi,
>
> Thanks for the great work Eric.
>
> I'm working to split the patch and pass it through some regression tests
> and performance benchmarking.
> Will send the split patch-set in few days.
This looks great, thanks Amir.
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Hannes Frederic Sowa @ 2014-09-30 12:03 UTC (permalink / raw)
To: David Laight
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174A0ECF@AcuExch.aculab.com>
On Di, 2014-09-30 at 11:18 +0000, David Laight wrote:
> From: Hannes Frederic
> > On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > > From: Hannes Frederic
> > > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > > From: Kees Cook
> > > > > >> This makes the size argument a const, since it is always populated by
> > > > > >> the caller.
> > > > > >
> > > > > > There is almost no point making parameters 'const.
> > > > > > ('const foo *' makes sense).
> > > > > >
> > > > > >> Additionally double-checks to make sure the copy_from_user
> > > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > > >>
> > > > > >> In function 'copy_from_user',
> > > > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > > >> ... copy_from_user() buffer size is not provably correct
> > > > > >
> > > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > > If it can't be too big you don't need the check.
> > > > >
> > > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > > which is const. Changing the function parameter to "const" means any
> > > > > changes made in the future where the incoming value isn't const, the
> > > > > compiler will throw a warning.
> > > >
> > > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > > __builtin_constant_p(var)). const in function argument only ensures that
> > > > the value cannot be modified in the function.
> > >
> > > You'd have to do something in the header file - nothing in the function
> > > body can do that check.
> >
> > Sure, it should work. You only need to make sure that gcc inlines the
> > function, so the value is constant (it is not enough that gcc knows the
> > value range, one specific constant is needed). So the simplest fix for
> > this is to specify __tun_chr_ioctl as __always_inline. ;)
>
> You are joking aren't you???
It would propagate the value from the compat and non-compat helper
functions to __tun_chr_ioctl and the __builtin_constant_p checks would
be true in the inlined copy_from/to_user, so no, I was not joking, but
because of my smiley I didn't considered this fix seriously.
> Look at the code.
I did.
> I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> to be less picky.
This might involve gcc changes. E.g. on my gcc I don't get any errors.
But by looking at the code it might be plausible wrong estimations are
being made.
But somehow I really cannot follow the original bug report. Looks like
Kees' __builtin_object_size is broken?
I also checked my tun.i file, I really use gcc __builtin_object_size.
Strange...
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH v2 net-next] mlx4: optimize xmit path
From: Amir Vadai @ 2014-09-30 12:01 UTC (permalink / raw)
To: Eric Dumazet, Or Gerlitz
Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
Eric Dumazet, John Fastabend, Linux Netdev List, Or Gerlitz
In-Reply-To: <1411964353.30721.6.camel@edumazet-glaptop2.roam.corp.google.com>
On 9/29/2014 7:19 AM, Eric Dumazet wrote:
> First I implemented skb->xmit_more support, and pktgen throughput
> went from ~5Mpps to ~10Mpps.
>
> Then, looking closely at this driver I found false sharing problems that
> should be addressed by this patch, as my pktgen now reaches 14.7 Mpps
> on a single TX queue, with a burst factor of 8.
>
> So this patch in a whole permits to improve raw performance on a single
> TX queue from about 5 Mpps to 14.9 Mpps.
>
> Note that if packets are below the inline_thold threshold (104 bytes),
> driver copies packets content into tx descriptor, and throughput
> is lowered to ~7.5 Mpps :
> -> We might reconsider inline strategy in a followup patch.
>
> I could split this patch into multiple components, but I prefer not
> spend days on this work.
Hi,
Thanks for the great work Eric.
I'm working to split the patch and pass it through some regression tests
and performance benchmarking.
Will send the split patch-set in few days.
Amir
^ permalink raw reply
* Re: Re: [PATCH net] gro: fix aggregation for skb using frag_list
From: Erez Shitrit @ 2014-09-30 11:51 UTC (permalink / raw)
To: erezsh, davem, eric.dumazet; +Cc: netdev
In-Reply-To: <07015a18f26f49518ceeabb35f2539ee@DB3PR05MB441.eurprd05.prod.outlook.com>
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Monday, September 29, 2014 10:19 PM
> To: eric.dumazet@gmail.com
> Cc: netdev@vger.kernel.org; Erez Shitrit
> Subject: Re: [PATCH net] gro: fix aggregation for skb using frag_list
>
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 29 Sep 2014 10:34:29 -0700
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> In commit 8a29111c7ca6 ("net: gro: allow to build full sized skb") I
>> added a regression for linear skb that traditionally force GRO to use
>> the frag_list fallback.
>>
>> Erez Shitrit found that at most two segments were aggregated and the
>> "if (skb_gro_len(p) != pinfo->gso_size)" test was failing.
>>
>> This is because pinfo at this spot still points to the last skb in the
>> chain, instead of the first one, where we find the correct gso_size
>> information.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Fixes: 8a29111c7ca6 ("net: gro: allow to build full sized skb")
>> Reported-by: Erez Shitrit <erezsh@mellanox.com>
> Applied and queued up for -stable, thanks Eric.
>
Tested. and it works for IPoIB driver. Thanks Eric.
Erez
^ permalink raw reply
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Eric Dumazet @ 2014-09-30 11:48 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, David S. Miller, Tom Herbert, Hannes Frederic Sowa,
Florian Westphal, Daniel Borkmann, Jamal Hadi Salim,
Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On Tue, 2014-09-30 at 10:53 +0200, Jesper Dangaard Brouer wrote:
> Based on DaveM's recent API work on dev_hard_start_xmit(), that allows
> sending/processing an entire skb list.
>
> This patch implements qdisc bulk dequeue, by allowing multiple packets
> to be dequeued in dequeue_skb().
>
> The optimization principle for this is two fold, (1) to amortize
> locking cost and (2) avoid expensive tailptr update for notifying HW.
> (1) Several packets are dequeued while holding the qdisc root_lock,
> amortizing locking cost over several packet. The dequeued SKB list is
> processed under the TXQ lock in dev_hard_start_xmit(), thus also
> amortizing the cost of the TXQ lock.
> (2) Further more, dev_hard_start_xmit() will utilize the skb->xmit_more
> API to delay HW tailptr update, which also reduces the cost per
> packet.
>
> One restriction of the new API is that every SKB must belong to the
> same TXQ. This patch takes the easy way out, by restricting bulk
> dequeue to qdisc's with the TCQ_F_ONETXQUEUE flag, that specifies the
> qdisc only have attached a single TXQ.
>
> Some detail about the flow; dev_hard_start_xmit() will process the skb
> list, and transmit packets individually towards the driver (see
> xmit_one()). In case the driver stops midway in the list, the
> remaining skb list is returned by dev_hard_start_xmit(). In
> sch_direct_xmit() this returned list is requeued by dev_requeue_skb().
>
> To avoid overshooting the HW limits, which results in requeuing, the
> patch limits the amount of bytes dequeued, based on the drivers BQL
> limits. In-effect bulking will only happen for BQL enabled drivers.
> Besides the bytelimit from BQL, also limit bulking to maximum 7
> packets to avoid any issues with available descriptor in HW and
> any HoL issues measured at 100Mbit/s.
>
> For now, as a conservative approach, don't bulk dequeue GSO and
> segmented GSO packets, because testing showed requeuing occuring
> with segmented GSO packets.
>
> Jointed work with Hannes, Daniel and Florian.
>
> Testing:
> --------
> Demonstrating the performance improvement of qdisc dequeue bulking, is
> tricky because the effect only "kicks-in" once the qdisc system have a
> backlog. Thus, for a backlog to form, we need either 1) to exceed wirespeed
> of the link or 2) exceed the capability of the device driver.
>
> For practical use-cases, the measureable effect of this will be a
> reduction in CPU usage
>
> 01-TCP_STREAM:
> --------------
> Testing effect for TCP involves disabling TSO and GSO, because TCP
> already benefit from bulking, via TSO and especially for GSO segmented
> packets. This patch view TSO/GSO as a seperate kind of bulking, and
> avoid further bulking of these packet types.
>
> The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66%
> less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).
>
> Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
> * Disabled bulking: 8.30% soft 88.75% idle
> * Enabled bulking: 7.80% soft 89.36% idle
>
> 02-UDP_STREAM
> -------------
> The measured perf diff benefit for UDP_STREAM were 5.26% less CPU used
> on calls to _raw_spin_lock(). UDP_STREAM with packet size -m 1472 (to
> avoid sending UDP/IP fragments).
>
> 03-trafgen driver test
> ----------------------
> The performance of the 10Gbit/s ixgbe driver is limited due to
> updating the HW ring-queue tail-pointer on every packet. As
> previously demonstrated with pktgen.
>
> Using trafgen to send RAW frames from userspace (via AF_PACKET), and
> forcing it through qdisc path (with option --qdisc-path and -t0),
> sending with 12 CPUs.
>
> I can demonstrate this driver layer limitation:
> * 12.8 Mpps with no qdisc bulking
> * 14.8 Mpps with qdisc bulking (full 10G-wirespeed)
>
> 04-netperf-wrapper
> ------------------
> I've designed some tests for the tool "netperf-wrapper" that tries to
> measure the Head-of-Line (HoL) blocking effect.
>
> The bulking "budget" limit of 7 packets are choosen, based on testing
> with netperf-wrapper. With driver igb at 100Mbit/s and a budget of
> max 7 packets I cannot measure any added HoL latency. Increasing this
> value to 8 packets, I observed a fluctuating added 0.24ms delay
> corrosponding to 3000 bytes at 100Mbit/s.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
>
> ---
> V5:
> - Changed "budget" limit to 7 packets (from 8)
> - Choosing to use skb->len, because BQL uses that
> - Added testing results to patch desc
>
> V4:
> - Patch rewritten in the Red Hat Neuchatel office jointed work with
> Hannes, Daniel and Florian.
> - Conservative approach of only using on BQL enabled drivers
> - No user tunable parameter, but limit bulking to 8 packets.
> - For now, avoid bulking GSO packets packets.
>
> V3:
> - Correct use of BQL
> - Some minor adjustments based on feedback.
> - Default setting only bulk dequeue 1 extra packet (2 packets).
>
> V2:
> - Restruct functions, split out functionality
> - Use BQL bytelimit to avoid overshooting driver limits
>
>
> include/net/sch_generic.h | 16 +++++++++++++++
> net/sched/sch_generic.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e65b8e0..0654dfa 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -6,6 +6,7 @@
> #include <linux/rcupdate.h>
> #include <linux/pkt_sched.h>
> #include <linux/pkt_cls.h>
> +#include <linux/dynamic_queue_limits.h>
> #include <net/gen_stats.h>
> #include <net/rtnetlink.h>
>
> @@ -111,6 +112,21 @@ static inline void qdisc_run_end(struct Qdisc *qdisc)
> qdisc->__state &= ~__QDISC___STATE_RUNNING;
> }
>
> +static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
> +{
> + return qdisc->flags & TCQ_F_ONETXQUEUE;
> +}
> +
> +static inline int qdisc_avail_bulklimit(const struct netdev_queue *txq)
> +{
> +#ifdef CONFIG_BQL
> + /* Non-BQL migrated drivers will return 0, too. */
> + return dql_avail(&txq->dql);
> +#else
> + return 0;
> +#endif
> +}
> +
> static inline bool qdisc_is_throttled(const struct Qdisc *qdisc)
> {
> return test_bit(__QDISC_STATE_THROTTLED, &qdisc->state) ? true : false;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 11b28f6..0fa8f0e 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -56,6 +56,42 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> return 0;
> }
>
> +static struct sk_buff *try_bulk_dequeue_skb(struct Qdisc *q,
> + struct sk_buff *head_skb,
> + int bytelimit)
> +{
> + struct sk_buff *skb, *tail_skb = head_skb;
> + int budget = 7; /* For now, conservatively choosen limit */
> +
> + while (bytelimit > 0 && --budget > 0) {
> + /* For now, don't bulk dequeue GSO (or GSO segmented) pkts */
> + if (tail_skb->next || skb_is_gso(tail_skb))
> + break;
> +
> + skb = q->dequeue(q);
> + if (!skb)
> + break;
> +
> + bytelimit -= skb->len; /* covers GSO len */
> + skb = validate_xmit_skb(skb, qdisc_dev(q));
> + if (!skb)
> + break;
> +
> + /* "skb" can be a skb list after validate call above
> + * (GSO segmented), but it is okay to append it to
> + * current tail_skb->next, because next round will exit
> + * in-case "tail_skb->next" is a skb list.
> + */
> + tail_skb->next = skb;
> + tail_skb = skb;
> + }
> +
> + return head_skb;
> +}
> +
> +/* Note that dequeue_skb can possibly return a SKB list (via skb->next).
> + * A requeued skb (via q->gso_skb) can also be a SKB list.
> + */
> static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> {
> struct sk_buff *skb = q->gso_skb;
> @@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> } else
> skb = NULL;
> } else {
> - if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
> + if (!(q->flags & TCQ_F_ONETXQUEUE) ||
> + !netif_xmit_frozen_or_stopped(txq)) {
> + int bytelimit = qdisc_avail_bulklimit(txq);
> +
> skb = q->dequeue(q);
> - if (skb)
> + if (skb) {
> + bytelimit -= skb->len;
> skb = validate_xmit_skb(skb, qdisc_dev(q));
> + }
> + if (skb && qdisc_may_bulk(q))
> + skb = try_bulk_dequeue_skb(q, skb, bytelimit);
> }
> }
Okay...
In my opinion, you guys missed some opportunities :
1) Ideally the algo should try to call validate_xmit_skb() outside of
the locks (qdisc lock or device lock), because eventually doing checksum
computation or full segmentation should allow other cpus doing enqueues.
2) TSO support. Have you ideas how to perform this ?
^ permalink raw reply
* Re: [PATCH] netfilter: fix nf_conn_nat->masq_index visibility
From: Pablo Neira Ayuso @ 2014-09-30 11:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: netdev, Arturo Borrero Gonzalez, davem, linux-kernel,
linux-arm-kernel, netfilter-devel, coreteam, Patrick McHardy,
Jozsef Kadlecsik
In-Reply-To: <2670668.vq09CjLWH3@wuerfel>
On Tue, Sep 30, 2014 at 01:27:50PM +0200, Arnd Bergmann wrote:
> A recent change introduced the NF_NAT_MASQUERADE_IPV4/6 symbols and now
> builds the masquerading code based on this symbol rather than the
> IP_NF_TARGET_MASQUERADE symbol, however the nf_nat.h header file
> still uses the old symbol, which leads to a build error when that
> is disabled:
>
> nf_nat_masquerade_ipv4.c: In function 'nf_nat_masquerade_ipv4':
> nf_nat_masquerade_ipv4.c:59:5: error: 'struct nf_conn_nat' has no member named 'masq_index'
>
> This changes the header file to do the respective change, and move
> to using the IS_ENABLED() macro in the process, to improve readability
> of the check
David pulled a patch to resolve this yesterday:
http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=67981fefb20e717cea55b42f9081a833fa46b3be
Thanks anyway.
^ permalink raw reply
* RE: [PATCH net-next] r8169:add support for RTL8168EP
From: Hau @ 2014-09-30 11:37 UTC (permalink / raw)
To: Francois Romieu
Cc: netdev@vger.kernel.org, nic_swsd, linux-kernel@vger.kernel.org
In-Reply-To: <20140927203921.GB28553@electric-eye.fr.zoreil.com>
I will spilt this patch and submit again.
Thanks,
Hau
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Sunday, September 28, 2014 4:39 AM
To: Hau
Cc: netdev@vger.kernel.org; nic_swsd; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] r8169:add support for RTL8168EP
Chun-Hao Lin <hau@realtek.com> :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
>
> This patch add support for this chip.
It does more than that.
Did Hayes review it ?
> Signed-off-by: Chun-Hao Lin <hau@realtek.com>
> ---
> drivers/net/ethernet/realtek/r8169.c | 611
> +++++++++++++++++++++++++++++------
> 1 file changed, 514 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
> RTL_GIGA_MAC_VER_46,
> RTL_GIGA_MAC_VER_47,
> RTL_GIGA_MAC_VER_48,
> + RTL_GIGA_MAC_VER_49,
> + RTL_GIGA_MAC_VER_50,
> + RTL_GIGA_MAC_VER_51,
> RTL_GIGA_MAC_NONE = 0xff,
> };
>
> @@ -302,6 +305,15 @@ static const struct {
> [RTL_GIGA_MAC_VER_48] =
> _R("RTL8107e", RTL_TD_1, FIRMWARE_8107E_2,
> JUMBO_1K, false),
> + [RTL_GIGA_MAC_VER_49] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_50] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_51] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> };
> #undef _R
>
> @@ -400,6 +412,10 @@ enum rtl_registers {
> FuncEvent = 0xf0,
> FuncEventMask = 0xf4,
> FuncPresetState = 0xf8,
> + IBCR0 = 0xf8,
> + IBCR2 = 0xf9,
> + IBIMR0 = 0xfa,
> + IBISR0 = 0xfb,
> FuncForceEvent = 0xfc,
> };
>
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
> #define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MSIX (0x01 << ERIAR_TYPE_SHIFT)
> #define ERIAR_ASF (0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB (0x02 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MASK_SHIFT 12
> #define ERIAR_MASK_0001 (0x1 << ERIAR_MASK_SHIFT)
> #define ERIAR_MASK_0011 (0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = { \
> \
> static bool name ## _check(struct rtl8169_private *tp)
>
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate patch. You are adding extra noise and thus making this stuff harder to review than it should be.
> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20) ?
> - RTL_R32(OCPDR) : ~0;
> -}
(this one is modified)
> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
> u32 data) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPDR, data);
> - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - rtl_udelay_loop_wait_low(tp, &rtl_ocpar_cond, 100, 20);
> -}
(this one is modified)
> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate patch.
> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd) -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W8(ERIDR, cmd);
> - RTL_W32(ERIAR, 0x800010e8);
> - msleep(2);
> -
> - if (!rtl_udelay_loop_wait_low(tp, &rtl_eriar_cond, 100, 5))
> - return;
> -
> - ocp_write(tp, 0x1, 0x30, 0x00000001);
> -}
This one is modified but it is also modified for the existing 8168DP.
Mantra: please do it in a separate patch.
> -
> #define OOB_CMD_RESET 0x00
> #define OOB_CMD_DRIVER_START 0x05
> #define OOB_CMD_DRIVER_STOP 0x06
>
> -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp) -{
> - return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
> -}
> -
> -DECLARE_RTL_COND(rtl_ocp_read_cond)
> -{
> - u16 reg;
> -
> - reg = rtl8168_get_ocp_reg(tp);
> -
> - return ocp_read(tp, 0x0f, reg) & 0x00000800;
> -}
(this one is simply moved around)
> -
> -static void rtl8168_driver_start(struct rtl8169_private *tp) -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> -
> - rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
> -}
(this one is modified)
> -
> -static void rtl8168_driver_stop(struct rtl8169_private *tp) -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_STOP);
> -
> - rtl_msleep_loop_wait_low(tp, &rtl_ocp_read_cond, 10, 10);
> -}
(this one is modified)
> -
> -static int r8168dp_check_dash(struct rtl8169_private *tp) -{
> - u16 reg = rtl8168_get_ocp_reg(tp);
> -
> - return (ocp_read(tp, 0x0f, reg) & 0x00008000) ? 1 : 0;
> -}
(this one is modified)
[...]
> @@ -1329,6 +1277,45 @@ static void rtl_w1w0_eri(struct rtl8169_private *tp, int addr, u32 mask, u32 p,
> rtl_eri_write(tp, addr, mask, (val & ~m) | p, type); }
>
> +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg) {
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + return rtl_eri_read(tp, reg, ERIAR_OOB);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> + return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20)
> + ? RTL_R32(OCPDR) : ~0;
Please:
- use a local "bool rc" variable for rtl_udelay_loop_wait_high status
so that you can line things correctly.
- use a switch case
- croak in the default case
> + }
> +
> + return ~0;
> +}
> +
> +static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
> +u32 data) {
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + rtl_eri_write(tp, reg, ((u32)mask & 0x0f) << ERIAR_MASK_SHIFT,
> + data, ERIAR_OOB);
It should line up after the opening parenthesis in "rtl_eri_write("
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPDR, data);
> + RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 |
> + (reg & 0x0fff));
It should line up after the opening parenthesis in "RTL_W32("
Please:
- use a switch case
- croak in the default case
You may state in a comment that this function is only designed for 8168DP and derivatives (whence the reduced switch case).
[...]
> @@ -1361,6 +1348,103 @@ static u8 rtl8168d_efuse_read(struct
> rtl8169_private *tp, int reg_addr)
[...]
> +static void rtl8168_driver_start(struct rtl8169_private *tp) {
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + u32 tmp;
> +
> + ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
> + tmp = ocp_read(tp, 0x01, 0x30);
> + tmp |= 0x01;
> + ocp_write(tp, 0x01, 0x30, tmp);
> + rtl_msleep_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10, 10);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> + rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
switch case + croak on default please.
[snip]
> @@ -3724,6 +3819,143 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp)
> rtl_writephy(tp, 0x1f, 0x0000);
> }
>
> +static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp) {
> + rtl_apply_firmware(tp);
?
You did not specify any firmware information in rtl_chip_infos.
It some firmware is supposed to help, please be sure to add the relevant #define FIRMWARE_8xyz as well.
[...]
> +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp) {
> + rtl_apply_firmware(tp);
See above.
[...]
> @@ -4265,7 +4511,7 @@ static void r810x_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_47:
> case RTL_GIGA_MAC_VER_48:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> break;
It's welcome but it should be done in a different patch.
[...]
> @@ -4339,8 +4585,11 @@ static void r8168_pll_power_down(struct
> rtl8169_private *tp)
>
> if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) &&
> - r8168dp_check_dash(tp)) {
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
> return;
Unrelated behavior change for RTL_GIGA_MAC_VER_2[78]. Different patch.
> }
>
> @@ -4369,12 +4618,16 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_33:
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0x00000000,
> 0xfc000000, ERIAR_EXGMAC);
> + RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
Unrelated behavior change for RTL_GIGA_MAC_VER_4[01] (8168g, huh ?).
-> Different patch.
[...]
> @@ -4395,10 +4648,14 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
See above.
> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0xfc000000,
> 0x00000000, ERIAR_EXGMAC);
See above.
[...]
> @@ -7366,9 +7766,13 @@ static void rtl_remove_one(struct pci_dev *pdev)
> struct net_device *dev = pci_get_drvdata(pdev);
> struct rtl8169_private *tp = netdev_priv(dev);
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
It does not line up correctly (one space excess).
Unrelated behavior change for RTL_GIGA_MAC_VER_{27, 28, 31}. Different patch.
> rtl8168_driver_stop(tp);
> }
>
[...]
> @@ -7703,11 +8113,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (tp->mac_version == RTL_GIGA_MAC_VER_45 ||
> tp->mac_version == RTL_GIGA_MAC_VER_46 ||
> tp->mac_version == RTL_GIGA_MAC_VER_47 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_48) {
> + tp->mac_version == RTL_GIGA_MAC_VER_48 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> u16 mac_addr[3];
>
> - *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xE0, ERIAR_EXGMAC);
> - *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xE4, ERIAR_EXGMAC);
> + *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> + *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);
It's welcome but it brings noise in the current patch. Different patch.
>
> if (is_valid_ether_addr((u8 *)mac_addr))
> rtl_rar_set(tp, (u8 *)mac_addr);
> @@ -7780,9 +8193,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> rtl_chip_infos[chipset].jumbo_tx_csum ? "ok" : "ko");
> }
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
See above. Unrelated change, etc.
> rtl8168_driver_start(tp);
> }
--
Ueimor
------Please consider the environment before printing this e-mail.
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Michael S. Tsirkin @ 2014-09-30 11:29 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, David S. Miller, Jason Wang, Zhi Yong Wu, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger, Network Development
In-Reply-To: <CAGXu5j+x5pg6-9y72o=mE9TiNEyFqTTE=o8ETh1Ao8R20KNRwA@mail.gmail.com>
On Mon, Sep 29, 2014 at 12:48:47PM -0700, Kees Cook wrote:
> On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
> >> This makes the size argument a const, since it is always populated by
> >> the caller. Additionally double-checks to make sure the copy_from_user
> >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> >>
> >> In function 'copy_from_user',
> >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >> ... copy_from_user() buffer size is not provably correct
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > What exactly is the issue here?
> > __tun_chr_ioctl is called with sizeof(struct compat_ifreq)
> > or sizeof (struct ifreq) as the last argument.
>
> Correct. There is no vulnerability here; I am attempting to both make
> the code more defensive to future changes, and to keep
> CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.
>
> > So this looks like a false positive, but
> > CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
> > to avoid false positives.
>
> The support in GCC is currently a bit faulty, and it seems that it
> didn't notice the two callers were static values, so instead, adding
> an explicit test keeps it happy.
>
> > On which architecture is this?
>
> This is on x86, but with CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> correctly enabled (gcc after 4.6 broke its ability to correctly
> optimize), which I've been playing with trying to get gcc working
> again. I sent the patch because it seems like it's a reasonable
> defensive change to make.
For the BUG_ON - I guess it's reasonable but we'll just break it again
by random code changes in the future. And when we do, I don't cherish
the thought of using trial and error to find a way to shut up the
warning again.
If gcc can't be fixed, something explicit like the uninitialized_var
is needed.
For const as an argument - is it also about gcc bugs?
We don't usually do this in kernel so if it's useful,
it has to be documented.
Finally, why are you switching the type from int to size_t?
Pls document the reason in the commit log.
> If you want to look more deeply, there's some background here:
> https://code.google.com/p/chromium/issues/detail?id=371036
>
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=gcc-bug&id=92dd7154932d8775a05dfd3de5564124c05a4150
>
> Thanks,
>
> -Kees
I think what you are trying to do is really useful, but
I doubt making random code changes that happen to make a
specific gcc version happy is maintainable.
HTH
Thanks!
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* [PATCH] netfilter: fix nf_conn_nat->masq_index visibility
From: Arnd Bergmann @ 2014-09-30 11:27 UTC (permalink / raw)
To: netdev
Cc: Arturo Borrero Gonzalez, Pablo Neira Ayuso, davem, linux-kernel,
linux-arm-kernel, netfilter-devel, coreteam, Patrick McHardy,
Jozsef Kadlecsik
A recent change introduced the NF_NAT_MASQUERADE_IPV4/6 symbols and now
builds the masquerading code based on this symbol rather than the
IP_NF_TARGET_MASQUERADE symbol, however the nf_nat.h header file
still uses the old symbol, which leads to a build error when that
is disabled:
nf_nat_masquerade_ipv4.c: In function 'nf_nat_masquerade_ipv4':
nf_nat_masquerade_ipv4.c:59:5: error: 'struct nf_conn_nat' has no member named 'masq_index'
This changes the header file to do the respective change, and move
to using the IS_ENABLED() macro in the process, to improve readability
of the check
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 8dd33cc93ec92 ("netfilter: nf_nat: generalize IPv4 masquerading support for nf_tables")
---
found while doing ARM randconfig tests
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index a71dd333ac68..68f0953c86c3 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -32,10 +32,8 @@ struct nf_conn_nat {
struct hlist_node bysource;
struct nf_conn *ct;
union nf_conntrack_nat_help help;
-#if defined(CONFIG_IP_NF_TARGET_MASQUERADE) || \
- defined(CONFIG_IP_NF_TARGET_MASQUERADE_MODULE) || \
- defined(CONFIG_IP6_NF_TARGET_MASQUERADE) || \
- defined(CONFIG_IP6_NF_TARGET_MASQUERADE_MODULE)
+#if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV4) || \
+ IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV6)
int masq_index;
#endif
};
@@ -56,7 +54,7 @@ int nf_nat_used_tuple(const struct nf_conntrack_tuple *tuple,
static inline struct nf_conn_nat *nfct_nat(const struct nf_conn *ct)
{
-#if defined(CONFIG_NF_NAT) || defined(CONFIG_NF_NAT_MODULE)
+#if IS_ENABLED(CONFIG_NF_NAT)
return nf_ct_ext_find(ct, NF_CT_EXT_NAT);
#else
return NULL;
@@ -68,8 +66,8 @@ static inline bool nf_nat_oif_changed(unsigned int hooknum,
struct nf_conn_nat *nat,
const struct net_device *out)
{
-#if IS_ENABLED(CONFIG_IP_NF_TARGET_MASQUERADE) || \
- IS_ENABLED(CONFIG_IP6_NF_TARGET_MASQUERADE)
+#if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV4) || \
+ IS_ENABLED(CONFIG_NF_NAT_MASQUERADE_IPV6)
return nat->masq_index && hooknum == NF_INET_POST_ROUTING &&
CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL &&
nat->masq_index != out->ifindex;
^ permalink raw reply related
* RE: [PATCH] tun: make sure interface usage can not overflow
From: David Laight @ 2014-09-30 11:18 UTC (permalink / raw)
To: 'Hannes Frederic Sowa'
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <1412075023.7155.21.camel@localhost>
From: Hannes Frederic
> On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> > From: Hannes Frederic
> > > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > > From: Kees Cook
> > > > >> This makes the size argument a const, since it is always populated by
> > > > >> the caller.
> > > > >
> > > > > There is almost no point making parameters 'const.
> > > > > ('const foo *' makes sense).
> > > > >
> > > > >> Additionally double-checks to make sure the copy_from_user
> > > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > > >>
> > > > >> In function 'copy_from_user',
> > > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > > >> ... copy_from_user() buffer size is not provably correct
> > > > >
> > > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > > If it can't be too big you don't need the check.
> > > >
> > > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > > which is const. Changing the function parameter to "const" means any
> > > > changes made in the future where the incoming value isn't const, the
> > > > compiler will throw a warning.
> > >
> > > Hmmm, I think you want something like BUILD_BUG_ON(!
> > > __builtin_constant_p(var)). const in function argument only ensures that
> > > the value cannot be modified in the function.
> >
> > You'd have to do something in the header file - nothing in the function
> > body can do that check.
>
> Sure, it should work. You only need to make sure that gcc inlines the
> function, so the value is constant (it is not enough that gcc knows the
> value range, one specific constant is needed). So the simplest fix for
> this is to specify __tun_chr_ioctl as __always_inline. ;)
You are joking aren't you???
Look at the code.
I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
to be less picky.
David
^ permalink raw reply
* [PATCH iproute2] iplink: do not require assigning negative ifindex at link creation
From: Atzm Watanabe @ 2014-09-30 11:10 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Stephen Hemminger
Since commit 3c682146aeff, iplink requires assigning negative
ifindex (-1) to the kernel when creating interface without
specifying index.
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
---
ip/iplink.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ip/iplink.c b/ip/iplink.c
index cb9c870..de6b2a9 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -689,7 +689,10 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
addattr_l(&req.n, sizeof(req), IFLA_LINK, &ifindex, 4);
}
- req.i.ifi_index = index;
+ if (index <= 0)
+ req.i.ifi_index = 0;
+ else
+ req.i.ifi_index = index;
}
if (name) {
--
1.8.1.5
^ permalink raw reply related
* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jamal Hadi Salim @ 2014-09-30 11:07 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
Eric Dumazet, Hannes Frederic Sowa, Florian Westphal,
Daniel Borkmann
Cc: Alexander Duyck, John Fastabend, Dave Taht, toke
In-Reply-To: <20140930085114.24043.81310.stgit@dragon>
On 09/30/14 04:53, Jesper Dangaard Brouer wrote:
[..]
> To avoid overshooting the HW limits, which results in requeuing, the
> patch limits the amount of bytes dequeued, based on the drivers BQL
> limits. In-effect bulking will only happen for BQL enabled drivers.
> Besides the bytelimit from BQL, also limit bulking to maximum 7
> packets to avoid any issues with available descriptor in HW and
> any HoL issues measured at 100Mbit/s.
>
[..]
>
> The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66%
> less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).
>
> Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
> * Disabled bulking: 8.30% soft 88.75% idle
> * Enabled bulking: 7.80% soft 89.36% idle
>
I know you have put a lot of hard work here and i hate to do
this to you, but:
The base test case is to surely *not to allow* the bulk code to be
executed at all. i.e when you say "disabled bulking" it
should mean not calling qdisc_may_bulk or qdisc_avail_bulklimit()
because that code was not there initially. My gut feeling is you
will find that your numbers for "Disabled bulking" to be a lot lower
than you show.
This is because no congestion likely happens at 24x TCP_STREAM
running at 10G with a modern day cpu and therefore you will end up
consuming more cpu.
Note, there are benefits as you have shown - but i would not
consider those to be standard use cases (actully one which would
have shown clear win is the VM thing Rusty was after). For this reason
my view is that i should be able to disable via ifdef bulking (yes, I
know DaveM hates ifdefs ;->).
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Hannes Frederic Sowa @ 2014-09-30 11:03 UTC (permalink / raw)
To: David Laight
Cc: Kees Cook, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D174A0C5C@AcuExch.aculab.com>
On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
> From: Hannes Frederic
> > On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> > > On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > > > From: Kees Cook
> > > >> This makes the size argument a const, since it is always populated by
> > > >> the caller.
> > > >
> > > > There is almost no point making parameters 'const.
> > > > ('const foo *' makes sense).
> > > >
> > > >> Additionally double-checks to make sure the copy_from_user
> > > >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> > > >>
> > > >> In function 'copy_from_user',
> > > >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> > > >> ... copy_from_user() buffer size is not provably correct
> > > >
> > > > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > > > If it can't be too big you don't need the check.
> > >
> > > The ifreq_len comes from the callers, and is the output of "sizeof"
> > > which is const. Changing the function parameter to "const" means any
> > > changes made in the future where the incoming value isn't const, the
> > > compiler will throw a warning.
> >
> > Hmmm, I think you want something like BUILD_BUG_ON(!
> > __builtin_constant_p(var)). const in function argument only ensures that
> > the value cannot be modified in the function.
>
> You'd have to do something in the header file - nothing in the function
> body can do that check.
Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH 2/7] netfilter: Convert print_tuple functions to return void
From: Petr Mladek @ 2014-09-30 10:22 UTC (permalink / raw)
To: Joe Perches
Cc: Steven Rostedt, Al Viro, Andrew Morton, Linus Torvalds,
Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <f2e8cf8df433a197daa62cbaf124c900c708edc7.1412031505.git.joe@perches.com>
On Mon 29-09-14 16:08:22, Joe Perches wrote:
> Since adding a new function to seq_file (seq_is_full)
> there isn't any value for functions called from seq_show to
> return anything. Remove the int returns of the various
> print_tuple/<foo>_print_tuple functions.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> include/net/netfilter/nf_conntrack_core.h | 2 +-
> include/net/netfilter/nf_conntrack_l3proto.h | 4 ++--
> include/net/netfilter/nf_conntrack_l4proto.h | 4 ++--
> net/netfilter/nf_conntrack_l3proto_generic.c | 5 ++---
> net/netfilter/nf_conntrack_proto_dccp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_generic.c | 5 ++---
> net/netfilter/nf_conntrack_proto_gre.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_sctp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_tcp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_udp.c | 10 +++++-----
> net/netfilter/nf_conntrack_proto_udplite.c | 10 +++++-----
> net/netfilter/nf_conntrack_standalone.c | 15 +++++++--------
> 12 files changed, 46 insertions(+), 49 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
> index cc0c188..f2f0fa3 100644
> --- a/include/net/netfilter/nf_conntrack_core.h
> +++ b/include/net/netfilter/nf_conntrack_core.h
> @@ -72,7 +72,7 @@ static inline int nf_conntrack_confirm(struct sk_buff *skb)
> return ret;
> }
>
> -int
> +void
> print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
> const struct nf_conntrack_l3proto *l3proto,
> const struct nf_conntrack_l4proto *proto);
> diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
> index adc1fa3..cdc920b 100644
> --- a/include/net/netfilter/nf_conntrack_l3proto.h
> +++ b/include/net/netfilter/nf_conntrack_l3proto.h
> @@ -38,8 +38,8 @@ struct nf_conntrack_l3proto {
> const struct nf_conntrack_tuple *orig);
>
> /* Print out the per-protocol part of the tuple. */
> - int (*print_tuple)(struct seq_file *s,
> - const struct nf_conntrack_tuple *);
> + void (*print_tuple)(struct seq_file *s,
> + const struct nf_conntrack_tuple *);
>
> /*
> * Called before tracking.
> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h
> index 4c8d573..fead8ee 100644
> --- a/include/net/netfilter/nf_conntrack_l4proto.h
> +++ b/include/net/netfilter/nf_conntrack_l4proto.h
> @@ -56,8 +56,8 @@ struct nf_conntrack_l4proto {
> u_int8_t pf, unsigned int hooknum);
>
> /* Print out the per-protocol part of the tuple. Return like seq_* */
> - int (*print_tuple)(struct seq_file *s,
> - const struct nf_conntrack_tuple *);
> + void (*print_tuple)(struct seq_file *s,
> + const struct nf_conntrack_tuple *);
>
> /* Print out the private part of the conntrack. */
> int (*print_conntrack)(struct seq_file *s, struct nf_conn *);
> diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
> index e7eb807..cf9ace7 100644
> --- a/net/netfilter/nf_conntrack_l3proto_generic.c
> +++ b/net/netfilter/nf_conntrack_l3proto_generic.c
> @@ -49,10 +49,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
> return true;
> }
>
> -static int generic_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return 0;
> }
>
> static int generic_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
> diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
> index cb372f9..40d96f9 100644
> --- a/net/netfilter/nf_conntrack_proto_dccp.c
> +++ b/net/netfilter/nf_conntrack_proto_dccp.c
> @@ -618,12 +618,12 @@ out_invalid:
> return -NF_ACCEPT;
> }
>
> -static int dccp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void dccp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.dccp.port),
> - ntohs(tuple->dst.u.dccp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.dccp.port),
> + ntohs(tuple->dst.u.dccp.port));
> }
>
> static int dccp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
> diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> index d25f293..0a3ded1 100644
> --- a/net/netfilter/nf_conntrack_proto_generic.c
> +++ b/net/netfilter/nf_conntrack_proto_generic.c
> @@ -39,10 +39,9 @@ static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int generic_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void generic_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return 0;
> }
>
> static unsigned int *generic_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
> index d566573..cd85336 100644
> --- a/net/netfilter/nf_conntrack_proto_gre.c
> +++ b/net/netfilter/nf_conntrack_proto_gre.c
> @@ -226,12 +226,12 @@ static bool gre_pkt_to_tuple(const struct sk_buff *skb, unsigned int dataoff,
> }
>
> /* print gre part of tuple */
> -static int gre_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void gre_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> - ntohs(tuple->src.u.gre.key),
> - ntohs(tuple->dst.u.gre.key));
> + seq_printf(s, "srckey=0x%x dstkey=0x%x ",
> + ntohs(tuple->src.u.gre.key),
> + ntohs(tuple->dst.u.gre.key));
> }
>
> /* print private data for conntrack */
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 1314d33..1b1d0e9 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -166,12 +166,12 @@ static bool sctp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int sctp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void sctp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.sctp.port),
> - ntohs(tuple->dst.u.sctp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.sctp.port),
> + ntohs(tuple->dst.u.sctp.port));
> }
>
> /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 44d1ea3..c2693e78 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -302,12 +302,12 @@ static bool tcp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int tcp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void tcp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.tcp.port),
> - ntohs(tuple->dst.u.tcp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.tcp.port),
> + ntohs(tuple->dst.u.tcp.port));
> }
>
> /* Print out the private part of the conntrack. */
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 9d7721c..6957281 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -63,12 +63,12 @@ static bool udp_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int udp_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void udp_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.udp.port),
> - ntohs(tuple->dst.u.udp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.udp.port),
> + ntohs(tuple->dst.u.udp.port));
> }
>
> static unsigned int *udp_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_proto_udplite.c b/net/netfilter/nf_conntrack_proto_udplite.c
> index 2750e6c..c5903d1 100644
> --- a/net/netfilter/nf_conntrack_proto_udplite.c
> +++ b/net/netfilter/nf_conntrack_proto_udplite.c
> @@ -71,12 +71,12 @@ static bool udplite_invert_tuple(struct nf_conntrack_tuple *tuple,
> }
>
> /* Print out the per-protocol part of the tuple. */
> -static int udplite_print_tuple(struct seq_file *s,
> - const struct nf_conntrack_tuple *tuple)
> +static void udplite_print_tuple(struct seq_file *s,
> + const struct nf_conntrack_tuple *tuple)
> {
> - return seq_printf(s, "sport=%hu dport=%hu ",
> - ntohs(tuple->src.u.udp.port),
> - ntohs(tuple->dst.u.udp.port));
> + seq_printf(s, "sport=%hu dport=%hu ",
> + ntohs(tuple->src.u.udp.port),
> + ntohs(tuple->dst.u.udp.port));
> }
>
> static unsigned int *udplite_get_timeouts(struct net *net)
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index cf65a1e..3c2ce5c 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -36,12 +36,13 @@
> MODULE_LICENSE("GPL");
>
> #ifdef CONFIG_NF_CONNTRACK_PROCFS
> -int
> +void
> print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
> const struct nf_conntrack_l3proto *l3proto,
> const struct nf_conntrack_l4proto *l4proto)
> {
> - return l3proto->print_tuple(s, tuple) || l4proto->print_tuple(s, tuple);
> + l3proto->print_tuple(s, tuple);
> + l4proto->print_tuple(s, tuple);
> }
> EXPORT_SYMBOL_GPL(print_tuple);
>
> @@ -202,9 +203,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> if (l4proto->print_conntrack && l4proto->print_conntrack(s, ct))
> goto release;
>
> - if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> - l3proto, l4proto))
> - goto release;
> + print_tuple(s, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple,
> + l3proto, l4proto);
To be precise, we should add:
if (seq_overflow(s))
goto release;
> if (seq_print_acct(s, ct, IP_CT_DIR_ORIGINAL))
> goto release;
> @@ -213,9 +213,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
> if (seq_printf(s, "[UNREPLIED] "))
> goto release;
>
> - if (print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> - l3proto, l4proto))
> - goto release;
> + print_tuple(s, &ct->tuplehash[IP_CT_DIR_REPLY].tuple,
> + l3proto, l4proto);
same here
> if (seq_print_acct(s, ct, IP_CT_DIR_REPLY))
> goto release;
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
From: Chen Gang @ 2014-09-30 10:10 UTC (permalink / raw)
To: David Vrabel, konrad.wilk, ian.campbell, wei.liu2,
boris.ostrovsky, bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev@vger.kernel.org, linux-pci, linux-scsi,
linux-kernel@vger.kernel.org
In-Reply-To: <542A7E0E.9050702@citrix.com>
On 9/30/14 17:55, David Vrabel wrote:
> On 26/09/14 17:36, Chen Gang wrote:
>> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
>> internally, so need not return any status value, then use 'void' instead
>> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>>
>> Also need be sure that all callers which check the return value must let
>> 'err' be 0.
>>
>> And also need change the related comments for xenbus_switch_state().
>
> Since this patch does not fix a bug and there is no unanimous agreement
> on the API change I'm not going to apply it (nor the previous version).
>
OK, at least for me, it is no problems.
But I still recommend to improve it in the future, it is not a good idea
to let all related things remain in current condition (for me, at lease
need some related code comments for it).
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: BCM4313 & brcmsmac & 3.12: only semi-working?
From: Arend van Spriel @ 2014-09-30 10:06 UTC (permalink / raw)
To: Maximilian Engelhardt
Cc: Michael Tokarev, Seth Forshee, brcm80211-dev-list, linux-wireless,
netdev
In-Reply-To: <2326963.udog5tteBt@eisbaer>
On 09/29/14 21:40, Maximilian Engelhardt wrote:
> On Monday 29 September 2014 15:44:03 Arend van Spriel wrote:
>> On 09/26/14 17:20, Michael Tokarev wrote:
>>> I can send it your way, -- guess it will be quite a bit costly,
>>> but I don't have any use for it anyway (short of throwing it
>>> away), and since I already spent significantly more money due
>>> to all this (whole thinkpad plus ssds and several wifi adaptors),
>>> this additional cost is just a small noize. But since that's
>>> 2nd card in a row, maybe there's something else in there, the
>>> prob is not in the card?
>>
>> Could be. Maybe some BIOS issue. Can you make some hi-res pictures of
>> the card and email them to me? If it is identical to what I already have
>> over here there is not much sense in sending it.
>>
>> Regards,
>> Arend
>
> Hi Arend,
>
> I just saw this thread on linux-wireless and wanted to answer as it might be
> of interest to you.
>
> I also own a BCM4313 wireless network card. About a year ago I reported some
> problems with reception strength which were then fixed after some time,
> debugging and testing passed. At that time I also did some throughput testing,
> but only had a 802.11g access-point to test. The results were not ideal, but
> also not too bad. So at that time I thought the issues were all more or less
> fixed and mostly fine. I also don't use wireless very much, so as long as things
> do work somehow acceptable I probably don't notice any problems immediately.
> So it comes that only about some month ago I noticed that the throughput I
> measured with my 11g access-point (about half the rate than with an atheros
> card on the same ap) is about the same on a 11n access-point where it should
> be much faster. I didn't experience any stalls, but that may also be that I
> didn't use the card enough to really notice them.
>
> I always wanted to report a bug because of the low throughout, but never got
> to it because of lack of time. I didn't want to provide a report saying just
> it doesn't work or it's slow without any data about it and a description how
> to reproduce it, as I think without this information a bug report is mostly
> useless.
>
> I also had a look at the kernel changelog of the brcmsm driver and notices
> there was little to no activity lately. Because of this I also wasn't sure if
> there is still someone interesting in fixing bugs for this device.
>
> As I was annoyed by the bad support for this card I decided it would be more
> easy and much less time consuming to simply buy another card than trying to
> get this fixed. So I bought a BCM43228 card, because it also supports 5 GHz.
> Only after it arrived I noticed that it was only supported by the 3.17+ kernel
> (not so much of a problem) and that it only seems to work in 802.11g mode
> (only slow speeds and no 5 GHz). At least I could use it in full 11g speeds,
> so it was a improvement.
>
> So I still don't have a card that does simply work. As I hope the missing
> support for my BCM43228 will hopefully be added some time in the future it
> probably would still be worth fixing the BCM4313 card as other users will also
> benefit from it.
>
> A friend of mine also has the same laptop than me and the same (or at least
> very similar) wireless card. He has told me he has also problem with stalls
> like Michael reported (if I remember the history of the thread correctly).
>
> So I'm not really sure where I should go from here. I can try to provide some
> debugging information as time permits, but I don't know how much time I will
> have for this in the future. Of course ideally I want to use the BCM43228 card
> with full support, as it can work on 5 GHz.
>
> Currently the BCM43228 card is plugged into my laptop, but I want to avoid
> swapping the cards more that very few times because the antenna connectors are
> only designed for very few (un)plug cycles.
>
> If it's of any information, my card is labeled BCM-BCM94313HMGB on the
> sticker, the laptop where it was originally is a ThinkPad Edge E135.
Thanks for taking time to chime in. This chipset is a pain in the....
The label info does help. You have a 4313 with internal PA for which
support was added later. The card that Michael has seems to have an
external PA. The initial iPA support patch broke things for everyone
with external PA so it was reverted. In the second round it was better,
but it seems Michael still had issues. As he mentioned BT issues and his
card shares the external PA between WLAN and BT I believe that there is
a BT-coex issue.
What is causing your 4313 to seemingly do 11g rates is hard to tell
without any debug info. I have that card over here, but in my cabled
setup it is doing 72Mbps, ie. 11n rate. I can run a rate-vs-range test
to see if there is an issue.
Thanks again and
Regards,
Arend
^ permalink raw reply
* Re: [PATCH] xen/xenbus: Use 'void' instead of 'int' for the return of xenbus_switch_state()
From: David Vrabel @ 2014-09-30 9:55 UTC (permalink / raw)
To: Chen Gang, konrad.wilk, ian.campbell, wei.liu2, boris.ostrovsky,
bhelgaas, jgross, yongjun_wei, mukesh.rathor
Cc: xen-devel, netdev@vger.kernel.org, linux-pci, linux-scsi,
linux-kernel@vger.kernel.org
In-Reply-To: <5425961A.5000604@gmail.com>
On 26/09/14 17:36, Chen Gang wrote:
> When xenbus_switch_state() fails, it will call xenbus_switch_fatal()
> internally, so need not return any status value, then use 'void' instead
> of 'int' for xenbus_switch_state() and __xenbus_switch_state().
>
> Also need be sure that all callers which check the return value must let
> 'err' be 0.
>
> And also need change the related comments for xenbus_switch_state().
Since this patch does not fix a bug and there is no unanimous agreement
on the API change I'm not going to apply it (nor the previous version).
David
^ permalink raw reply
* Re: [PATCH v1 5/5] driver-core: add driver asynchronous probe support
From: Tom Gundersen @ 2014-09-30 9:22 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, Michal Hocko, Greg KH, Dmitry Torokhov,
Takashi Iwai, Tejun Heo, Arjan van de Ven, Robert Milasan, werner,
Oleg Nesterov, hare, Benjamin Poirier, Santosh Rastapur, pmladek,
dbueso, LKML, Tetsuo Handa, Joseph Salisbury, Kay Sievers,
One Thousand Gnomes, Tim Gardner, Pierre Fersing, Andrew Morton,
Nagalakshmi Nandigama
In-Reply-To: <20140930022751.GA14081@wotan.suse.de>
On Tue, Sep 30, 2014 at 4:27 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Sun, Sep 28, 2014 at 07:07:24PM +0200, Tom Gundersen wrote:
>> On Fri, Sep 26, 2014 at 11:57 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> > Systemd has a general timeout for all workers currently set to 180
>> > seconds after which it will send a sigkill signal. Systemd now has a
>> > warning which is issued once it reaches 1/3 of the timeout. The original
>> > motivation for the systemd timeout was to help track device drivers
>> > which do not use asynch firmware loading on init() and the timeout was
>> > originally set to 30 seconds.
>>
>> Please note that the motivation for the timeout in systemd had nothing
>> to do with async firmware loading (that was just the case where
>> problems cropped up).
>
> *Part *of the original kill logic, according to the commit log, was actually
> due to the assumption that the issues observed *were* synchronous firmware
> loading on module init():
>
> commit e64fae5573e566ce4fd9b23c68ac8f3096603314
> Author: Kay Sievers <kay.sievers@vrfy.org>
> Date: Wed Jan 18 05:06:18 2012 +0100
>
> udevd: kill hanging event processes after 30 seconds
>
> Some broken kernel drivers load firmware synchronously in the module init
> path and block modprobe until the firmware request is fulfilled.
> <...>
This was a workaround to avoid a deadlock between udev and the kernel.
The 180 s timeout was already in place before this change, and was not
motivated by firmware loading. Also note that this patch was not about
"tracking device drivers", just about avoiding dead-lock.
> My point here is not to point fingers but to explain why we went on with
> this and how we failed to realize only until later that the driver core
> ran probe together with init. When a few folks pointed out the issues
> with the kill the issue was punted back to kernel developers and the
> assumption even among some kernel maintainers was that it was init paths
> with sync behaviour that was causing some delays and they were broken
> drivers. It is important to highlight these assumptions ended up setting
> us off on the wrong path for a while in a hunt to try to fix this issue
> either in driver or elsewhere.
Ok. I'm not sure the motivations for user-space changes is important
to include in the commit message, but if you do I'll try to clarify
things to avoid misunderstandings.
> Thanks for clarifying this, can you explain what issues could arise
> from making an exception to allowing kmod workers to hang around
> completing init + probe over a certain defined amount of time without
> being killed?
We could run out of udev workers and the whole boot would hang.
The way I see it, the current status from systemd's side is: our
short-term work-around is to increase the timeout, and at the moment
it appears no long-term solution is needed (i.e., it seems like the
right thing to do is to make sure insmod can be near instantaneous, it
appears people are working towards this goal, and so far no examples
have cropped up showing that it is fundamentally impossible (once/if
they do, we should of course revisit the problem)).
Cheers,
Tom
^ permalink raw reply
* [PATCH net-next 2/4] net/mlx4_core: New init and exit flow for mlx4_core
From: Or Gerlitz @ 2014-09-30 9:03 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Jack Morgenstein, Majd Dibbiny, Or Gerlitz
In-Reply-To: <1412067830-30438-1-git-send-email-ogerlitz@mellanox.com>
From: Majd Dibbiny <majd@mellanox.com>
In the new flow, we separate the pci initialization and teardown
from the initialization and teardown of the other resources.
__mlx4_init_one handles the pci resources initialization. It then
calls mlx4_load_one to initialize the remainder of the resources.
When removing a device, mlx4_remove_one is invoked. However, now
mlx4_remove_one calls mlx4_unload_one to free all the resources except the pci
resources. When mlx4_unload_one returns, mlx4_remove_one then frees the
pci resources.
The above separation will allow us to implement 'reset flow' in the future.
It will also enable more EQs for VFs and is a pre-step to the modern API to
enable/disable SRIOV.
Also added nvfs; an integer array of size MLX4_MAX_PORTS + 1; to the mlx4_dev
struct. This new field is used to avoid parsing the num_vfs module parameter
each time the mlx4_restart_one is called.
Signed-off-by: Majd Dibbiny <majd@mellanox.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx4/main.c | 393 ++++++++++++++++-------------
include/linux/mlx4/device.h | 1 +
2 files changed, 220 insertions(+), 174 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 4e9857b..f6c32a9 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -2259,116 +2259,18 @@ static void mlx4_free_ownership(struct mlx4_dev *dev)
iounmap(owner);
}
-static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
+static int mlx4_load_one(struct pci_dev *pdev, int pci_dev_data,
+ int total_vfs, int *nvfs, struct mlx4_priv *priv)
{
- struct mlx4_priv *priv;
struct mlx4_dev *dev;
+ unsigned sum = 0;
int err;
int port;
- int nvfs[MLX4_MAX_PORTS + 1] = {0, 0, 0};
- int prb_vf[MLX4_MAX_PORTS + 1] = {0, 0, 0};
- const int param_map[MLX4_MAX_PORTS + 1][MLX4_MAX_PORTS + 1] = {
- {2, 0, 0}, {0, 1, 2}, {0, 1, 2} };
- unsigned total_vfs = 0;
- int sriov_initialized = 0;
- unsigned int i;
+ int i;
int existing_vfs = 0;
- pr_info(DRV_NAME ": Initializing %s\n", pci_name(pdev));
-
- err = pci_enable_device(pdev);
- if (err) {
- dev_err(&pdev->dev, "Cannot enable PCI device, aborting\n");
- return err;
- }
-
- /* Due to requirement that all VFs and the PF are *guaranteed* 2 MACS
- * per port, we must limit the number of VFs to 63 (since their are
- * 128 MACs)
- */
- for (i = 0; i < sizeof(nvfs)/sizeof(nvfs[0]) && i < num_vfs_argc;
- total_vfs += nvfs[param_map[num_vfs_argc - 1][i]], i++) {
- nvfs[param_map[num_vfs_argc - 1][i]] = num_vfs[i];
- if (nvfs[i] < 0) {
- dev_err(&pdev->dev, "num_vfs module parameter cannot be negative\n");
- return -EINVAL;
- }
- }
- for (i = 0; i < sizeof(prb_vf)/sizeof(prb_vf[0]) && i < probe_vfs_argc;
- i++) {
- prb_vf[param_map[probe_vfs_argc - 1][i]] = probe_vf[i];
- if (prb_vf[i] < 0 || prb_vf[i] > nvfs[i]) {
- dev_err(&pdev->dev, "probe_vf module parameter cannot be negative or greater than num_vfs\n");
- return -EINVAL;
- }
- }
- if (total_vfs >= MLX4_MAX_NUM_VF) {
- dev_err(&pdev->dev,
- "Requested more VF's (%d) than allowed (%d)\n",
- total_vfs, MLX4_MAX_NUM_VF - 1);
- return -EINVAL;
- }
-
- for (i = 0; i < MLX4_MAX_PORTS; i++) {
- if (nvfs[i] + nvfs[2] >= MLX4_MAX_NUM_VF_P_PORT) {
- dev_err(&pdev->dev,
- "Requested more VF's (%d) for port (%d) than allowed (%d)\n",
- nvfs[i] + nvfs[2], i + 1,
- MLX4_MAX_NUM_VF_P_PORT - 1);
- return -EINVAL;
- }
- }
-
-
- /*
- * Check for BARs.
- */
- if (!(pci_dev_data & MLX4_PCI_DEV_IS_VF) &&
- !(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
- dev_err(&pdev->dev, "Missing DCS, aborting (driver_data: 0x%x, pci_resource_flags(pdev, 0):0x%lx)\n",
- pci_dev_data, pci_resource_flags(pdev, 0));
- err = -ENODEV;
- goto err_disable_pdev;
- }
- if (!(pci_resource_flags(pdev, 2) & IORESOURCE_MEM)) {
- dev_err(&pdev->dev, "Missing UAR, aborting\n");
- err = -ENODEV;
- goto err_disable_pdev;
- }
-
- err = pci_request_regions(pdev, DRV_NAME);
- if (err) {
- dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
- goto err_disable_pdev;
- }
-
- pci_set_master(pdev);
+ dev = &priv->dev;
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
- if (err) {
- dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA mask\n");
- err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
- if (err) {
- dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting\n");
- goto err_release_regions;
- }
- }
- err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
- if (err) {
- dev_warn(&pdev->dev, "Warning: couldn't set 64-bit consistent PCI DMA mask\n");
- err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
- if (err) {
- dev_err(&pdev->dev, "Can't set consistent PCI DMA mask, aborting\n");
- goto err_release_regions;
- }
- }
-
- /* Allow large DMA segments, up to the firmware limit of 1 GB */
- dma_set_max_seg_size(&pdev->dev, 1024 * 1024 * 1024);
-
- dev = pci_get_drvdata(pdev);
- priv = mlx4_priv(dev);
- dev->pdev = pdev;
INIT_LIST_HEAD(&priv->ctx_list);
spin_lock_init(&priv->ctx_lock);
@@ -2382,28 +2284,9 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
dev->rev_id = pdev->revision;
dev->numa_node = dev_to_node(&pdev->dev);
+
/* Detect if this device is a virtual function */
if (pci_dev_data & MLX4_PCI_DEV_IS_VF) {
- /* When acting as pf, we normally skip vfs unless explicitly
- * requested to probe them. */
- if (total_vfs) {
- unsigned vfs_offset = 0;
- for (i = 0; i < sizeof(nvfs)/sizeof(nvfs[0]) &&
- vfs_offset + nvfs[i] < extended_func_num(pdev);
- vfs_offset += nvfs[i], i++)
- ;
- if (i == sizeof(nvfs)/sizeof(nvfs[0])) {
- err = -ENODEV;
- goto err_free_dev;
- }
- if ((extended_func_num(pdev) - vfs_offset)
- > prb_vf[i]) {
- mlx4_warn(dev, "Skipping virtual function:%d\n",
- extended_func_num(pdev));
- err = -ENODEV;
- goto err_free_dev;
- }
- }
mlx4_warn(dev, "Detected virtual function - running in slave mode\n");
dev->flags |= MLX4_FLAG_SLAVE;
} else {
@@ -2413,11 +2296,10 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
err = mlx4_get_ownership(dev);
if (err) {
if (err < 0)
- goto err_free_dev;
+ return err;
else {
mlx4_warn(dev, "Multiple PFs not yet supported - Skipping PF\n");
- err = -EINVAL;
- goto err_free_dev;
+ return -EINVAL;
}
}
@@ -2429,7 +2311,8 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
GFP_KERNEL);
if (NULL == dev->dev_vfs) {
mlx4_err(dev, "Failed to allocate memory for VFs\n");
- err = 0;
+ err = -ENOMEM;
+ goto err_free_own;
} else {
atomic_inc(&pf_loading);
existing_vfs = pci_num_vf(pdev);
@@ -2445,13 +2328,11 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
mlx4_err(dev, "Failed to enable SR-IOV, continuing without SR-IOV (err = %d)\n",
err);
atomic_dec(&pf_loading);
- err = 0;
} else {
mlx4_warn(dev, "Running in master mode\n");
dev->flags |= MLX4_FLAG_SRIOV |
MLX4_FLAG_MASTER;
dev->num_vfs = total_vfs;
- sriov_initialized = 1;
}
}
}
@@ -2467,7 +2348,7 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
err = mlx4_reset(dev);
if (err) {
mlx4_err(dev, "Failed to reset HCA, aborting\n");
- goto err_rel_own;
+ goto err_sriov;
}
}
@@ -2517,34 +2398,46 @@ slave_start:
/* In master functions, the communication channel must be initialized
* after obtaining its address from fw */
if (mlx4_is_master(dev)) {
- unsigned sum = 0;
- err = mlx4_multi_func_init(dev);
- if (err) {
- mlx4_err(dev, "Failed to init master mfunc interface, aborting\n");
+ int ib_ports = 0;
+
+ mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_IB)
+ ib_ports++;
+
+ if (ib_ports &&
+ (num_vfs_argc > 1 || probe_vfs_argc > 1)) {
+ mlx4_err(dev,
+ "Invalid syntax of num_vfs/probe_vfs with IB port - single port VFs syntax is only supported when all ports are configured as ethernet\n");
+ err = -EINVAL;
+ goto err_close;
+ }
+ if (dev->caps.num_ports < 2 &&
+ num_vfs_argc > 1) {
+ err = -EINVAL;
+ mlx4_err(dev,
+ "Error: Trying to configure VFs on port 2, but HCA has only %d physical ports\n",
+ dev->caps.num_ports);
goto err_close;
}
- if (sriov_initialized) {
- int ib_ports = 0;
- mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_IB)
- ib_ports++;
+ memcpy(dev->nvfs, nvfs, sizeof(dev->nvfs));
- if (ib_ports &&
- (num_vfs_argc > 1 || probe_vfs_argc > 1)) {
- mlx4_err(dev,
- "Invalid syntax of num_vfs/probe_vfs with IB port - single port VFs syntax is only supported when all ports are configured as ethernet\n");
- err = -EINVAL;
- goto err_master_mfunc;
- }
- for (i = 0; i < sizeof(nvfs)/sizeof(nvfs[0]); i++) {
- unsigned j;
- for (j = 0; j < nvfs[i]; ++sum, ++j) {
- dev->dev_vfs[sum].min_port =
- i < 2 ? i + 1 : 1;
- dev->dev_vfs[sum].n_ports = i < 2 ? 1 :
- dev->caps.num_ports;
- }
+ for (i = 0; i < sizeof(dev->nvfs)/sizeof(dev->nvfs[0]); i++) {
+ unsigned j;
+
+ for (j = 0; j < dev->nvfs[i]; ++sum, ++j) {
+ dev->dev_vfs[sum].min_port = i < 2 ? i + 1 : 1;
+ dev->dev_vfs[sum].n_ports = i < 2 ? 1 :
+ dev->caps.num_ports;
}
}
+
+ /* In master functions, the communication channel
+ * must be initialized after obtaining its address from fw
+ */
+ err = mlx4_multi_func_init(dev);
+ if (err) {
+ mlx4_err(dev, "Failed to init master mfunc interface, aborting.\n");
+ goto err_close;
+ }
}
err = mlx4_alloc_eq_table(dev);
@@ -2565,7 +2458,7 @@ slave_start:
if (!mlx4_is_slave(dev)) {
err = mlx4_init_steering(dev);
if (err)
- goto err_free_eq;
+ goto err_disable_msix;
}
err = mlx4_setup_hca(dev);
@@ -2625,6 +2518,10 @@ err_steer:
if (!mlx4_is_slave(dev))
mlx4_clear_steering(dev);
+err_disable_msix:
+ if (dev->flags & MLX4_FLAG_MSI_X)
+ pci_disable_msix(pdev);
+
err_free_eq:
mlx4_free_eq_table(dev);
@@ -2641,9 +2538,6 @@ err_master_mfunc:
}
err_close:
- if (dev->flags & MLX4_FLAG_MSI_X)
- pci_disable_msix(pdev);
-
mlx4_close_hca(dev);
err_mfunc:
@@ -2657,17 +2551,151 @@ err_sriov:
if (dev->flags & MLX4_FLAG_SRIOV && !existing_vfs)
pci_disable_sriov(pdev);
-err_rel_own:
- if (!mlx4_is_slave(dev))
- mlx4_free_ownership(dev);
-
if (mlx4_is_master(dev) && dev->num_vfs)
atomic_dec(&pf_loading);
kfree(priv->dev.dev_vfs);
-err_free_dev:
- kfree(priv);
+err_free_own:
+ if (!mlx4_is_slave(dev))
+ mlx4_free_ownership(dev);
+
+ return err;
+}
+
+static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
+ struct mlx4_priv *priv)
+{
+ int err;
+ int nvfs[MLX4_MAX_PORTS + 1] = {0, 0, 0};
+ int prb_vf[MLX4_MAX_PORTS + 1] = {0, 0, 0};
+ const int param_map[MLX4_MAX_PORTS + 1][MLX4_MAX_PORTS + 1] = {
+ {2, 0, 0}, {0, 1, 2}, {0, 1, 2} };
+ unsigned total_vfs = 0;
+ unsigned int i;
+
+ pr_info(DRV_NAME ": Initializing %s\n", pci_name(pdev));
+
+ err = pci_enable_device(pdev);
+ if (err) {
+ dev_err(&pdev->dev, "Cannot enable PCI device, aborting\n");
+ return err;
+ }
+
+ /* Due to requirement that all VFs and the PF are *guaranteed* 2 MACS
+ * per port, we must limit the number of VFs to 63 (since their are
+ * 128 MACs)
+ */
+ for (i = 0; i < sizeof(nvfs)/sizeof(nvfs[0]) && i < num_vfs_argc;
+ total_vfs += nvfs[param_map[num_vfs_argc - 1][i]], i++) {
+ nvfs[param_map[num_vfs_argc - 1][i]] = num_vfs[i];
+ if (nvfs[i] < 0) {
+ dev_err(&pdev->dev, "num_vfs module parameter cannot be negative\n");
+ err = -EINVAL;
+ goto err_disable_pdev;
+ }
+ }
+ for (i = 0; i < sizeof(prb_vf)/sizeof(prb_vf[0]) && i < probe_vfs_argc;
+ i++) {
+ prb_vf[param_map[probe_vfs_argc - 1][i]] = probe_vf[i];
+ if (prb_vf[i] < 0 || prb_vf[i] > nvfs[i]) {
+ dev_err(&pdev->dev, "probe_vf module parameter cannot be negative or greater than num_vfs\n");
+ err = -EINVAL;
+ goto err_disable_pdev;
+ }
+ }
+ if (total_vfs >= MLX4_MAX_NUM_VF) {
+ dev_err(&pdev->dev,
+ "Requested more VF's (%d) than allowed (%d)\n",
+ total_vfs, MLX4_MAX_NUM_VF - 1);
+ err = -EINVAL;
+ goto err_disable_pdev;
+ }
+
+ for (i = 0; i < MLX4_MAX_PORTS; i++) {
+ if (nvfs[i] + nvfs[2] >= MLX4_MAX_NUM_VF_P_PORT) {
+ dev_err(&pdev->dev,
+ "Requested more VF's (%d) for port (%d) than allowed (%d)\n",
+ nvfs[i] + nvfs[2], i + 1,
+ MLX4_MAX_NUM_VF_P_PORT - 1);
+ err = -EINVAL;
+ goto err_disable_pdev;
+ }
+ }
+
+ /* Check for BARs. */
+ if (!(pci_dev_data & MLX4_PCI_DEV_IS_VF) &&
+ !(pci_resource_flags(pdev, 0) & IORESOURCE_MEM)) {
+ dev_err(&pdev->dev, "Missing DCS, aborting (driver_data: 0x%x, pci_resource_flags(pdev, 0):0x%lx)\n",
+ pci_dev_data, pci_resource_flags(pdev, 0));
+ err = -ENODEV;
+ goto err_disable_pdev;
+ }
+ if (!(pci_resource_flags(pdev, 2) & IORESOURCE_MEM)) {
+ dev_err(&pdev->dev, "Missing UAR, aborting\n");
+ err = -ENODEV;
+ goto err_disable_pdev;
+ }
+
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+ goto err_disable_pdev;
+ }
+
+ pci_set_master(pdev);
+
+ err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
+ if (err) {
+ dev_warn(&pdev->dev, "Warning: couldn't set 64-bit PCI DMA mask\n");
+ err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
+ if (err) {
+ dev_err(&pdev->dev, "Can't set PCI DMA mask, aborting\n");
+ goto err_release_regions;
+ }
+ }
+ err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
+ if (err) {
+ dev_warn(&pdev->dev, "Warning: couldn't set 64-bit consistent PCI DMA mask\n");
+ err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
+ if (err) {
+ dev_err(&pdev->dev, "Can't set consistent PCI DMA mask, aborting\n");
+ goto err_release_regions;
+ }
+ }
+
+ /* Allow large DMA segments, up to the firmware limit of 1 GB */
+ dma_set_max_seg_size(&pdev->dev, 1024 * 1024 * 1024);
+ /* Detect if this device is a virtual function */
+ if (pci_dev_data & MLX4_PCI_DEV_IS_VF) {
+ /* When acting as pf, we normally skip vfs unless explicitly
+ * requested to probe them.
+ */
+ if (total_vfs) {
+ unsigned vfs_offset = 0;
+
+ for (i = 0; i < sizeof(nvfs)/sizeof(nvfs[0]) &&
+ vfs_offset + nvfs[i] < extended_func_num(pdev);
+ vfs_offset += nvfs[i], i++)
+ ;
+ if (i == sizeof(nvfs)/sizeof(nvfs[0])) {
+ err = -ENODEV;
+ goto err_release_regions;
+ }
+ if ((extended_func_num(pdev) - vfs_offset)
+ > prb_vf[i]) {
+ dev_warn(&pdev->dev, "Skipping virtual function:%d\n",
+ extended_func_num(pdev));
+ err = -ENODEV;
+ goto err_release_regions;
+ }
+ }
+ }
+
+ err = mlx4_load_one(pdev, pci_dev_data, total_vfs, nvfs, priv);
+ if (err)
+ goto err_release_regions;
+ return 0;
err_release_regions:
pci_release_regions(pdev);
@@ -2682,6 +2710,7 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct mlx4_priv *priv;
struct mlx4_dev *dev;
+ int ret;
printk_once(KERN_INFO "%s", mlx4_version);
@@ -2690,13 +2719,18 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
return -ENOMEM;
dev = &priv->dev;
+ dev->pdev = pdev;
pci_set_drvdata(pdev, dev);
priv->pci_dev_data = id->driver_data;
- return __mlx4_init_one(pdev, id->driver_data);
+ ret = __mlx4_init_one(pdev, id->driver_data, priv);
+ if (ret)
+ kfree(priv);
+
+ return ret;
}
-static void __mlx4_remove_one(struct pci_dev *pdev)
+static void mlx4_unload_one(struct pci_dev *pdev)
{
struct mlx4_dev *dev = pci_get_drvdata(pdev);
struct mlx4_priv *priv = mlx4_priv(dev);
@@ -2775,8 +2809,6 @@ static void __mlx4_remove_one(struct pci_dev *pdev)
kfree(dev->caps.qp1_proxy);
kfree(dev->dev_vfs);
- pci_release_regions(pdev);
- pci_disable_device(pdev);
memset(priv, 0, sizeof(*priv));
priv->pci_dev_data = pci_dev_data;
priv->removed = 1;
@@ -2787,7 +2819,9 @@ static void mlx4_remove_one(struct pci_dev *pdev)
struct mlx4_dev *dev = pci_get_drvdata(pdev);
struct mlx4_priv *priv = mlx4_priv(dev);
- __mlx4_remove_one(pdev);
+ mlx4_unload_one(pdev);
+ pci_release_regions(pdev);
+ pci_disable_device(pdev);
kfree(priv);
pci_set_drvdata(pdev, NULL);
}
@@ -2796,11 +2830,22 @@ int mlx4_restart_one(struct pci_dev *pdev)
{
struct mlx4_dev *dev = pci_get_drvdata(pdev);
struct mlx4_priv *priv = mlx4_priv(dev);
- int pci_dev_data;
+ int nvfs[MLX4_MAX_PORTS + 1] = {0, 0, 0};
+ int pci_dev_data, err, total_vfs;
pci_dev_data = priv->pci_dev_data;
- __mlx4_remove_one(pdev);
- return __mlx4_init_one(pdev, pci_dev_data);
+ total_vfs = dev->num_vfs;
+ memcpy(nvfs, dev->nvfs, sizeof(dev->nvfs));
+
+ mlx4_unload_one(pdev);
+ err = mlx4_load_one(pdev, pci_dev_data, total_vfs, nvfs, priv);
+ if (err) {
+ mlx4_err(dev, "%s: ERROR: mlx4_load_one failed, pci_name=%s, err=%d\n",
+ __func__, pci_name(pdev), err);
+ return err;
+ }
+
+ return err;
}
static const struct pci_device_id mlx4_pci_table[] = {
@@ -2854,7 +2899,7 @@ MODULE_DEVICE_TABLE(pci, mlx4_pci_table);
static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
pci_channel_state_t state)
{
- __mlx4_remove_one(pdev);
+ mlx4_unload_one(pdev);
return state == pci_channel_io_perm_failure ?
PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_NEED_RESET;
@@ -2866,7 +2911,7 @@ static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
struct mlx4_priv *priv = mlx4_priv(dev);
int ret;
- ret = __mlx4_init_one(pdev, priv->pci_dev_data);
+ ret = __mlx4_init_one(pdev, priv->pci_dev_data, priv);
return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
}
@@ -2880,7 +2925,7 @@ static struct pci_driver mlx4_driver = {
.name = DRV_NAME,
.id_table = mlx4_pci_table,
.probe = mlx4_init_one,
- .shutdown = __mlx4_remove_one,
+ .shutdown = mlx4_unload_one,
.remove = mlx4_remove_one,
.err_handler = &mlx4_err_handler,
};
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index 03b5608..b2f8ab9 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -707,6 +707,7 @@ struct mlx4_dev {
u64 regid_promisc_array[MLX4_MAX_PORTS + 1];
u64 regid_allmulti_array[MLX4_MAX_PORTS + 1];
struct mlx4_vf_dev *dev_vfs;
+ int nvfs[MLX4_MAX_PORTS + 1];
};
struct mlx4_eqe {
--
1.7.1
^ permalink raw reply related
* [PATCH net-next 3/4] net/mlx4_core: Protect QUERY_PORT wrapper from untrusted guests
From: Or Gerlitz @ 2014-09-30 9:03 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Amir Vadai, Jack Morgenstein, Or Gerlitz
In-Reply-To: <1412067830-30438-1-git-send-email-ogerlitz@mellanox.com>
From: Jack Morgenstein <jackm@dev.mellanox.co.il>
The function mlx4_QUERY_PORT_wrapper implements only the
QUERY_PORT "general" case (opcode modifier = 0).
Verify that the opcode modifier is zero, and also that the
input modifier contains only the port number in bits 0..7
(all other bits should be zero).
Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx4/fw.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/fw.c b/drivers/net/ethernet/mellanox/mlx4/fw.c
index 13b2e4a..2e88a23 100644
--- a/drivers/net/ethernet/mellanox/mlx4/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx4/fw.c
@@ -982,8 +982,13 @@ int mlx4_QUERY_PORT_wrapper(struct mlx4_dev *dev, int slave,
if (port < 0)
return -EINVAL;
- vhcr->in_modifier = (vhcr->in_modifier & ~0xFF) |
- (port & 0xFF);
+ /* Protect against untrusted guests: enforce that this is the
+ * QUERY_PORT general query.
+ */
+ if (vhcr->op_modifier || vhcr->in_modifier & ~0xFF)
+ return -EINVAL;
+
+ vhcr->in_modifier = port;
err = mlx4_cmd_box(dev, 0, outbox->dma, vhcr->in_modifier, 0,
MLX4_CMD_QUERY_PORT, MLX4_CMD_TIME_CLASS_B,
--
1.7.1
^ permalink raw reply related
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