* [PATCH] devicetree: net: micrel-ksz90x1.txt: Properly explain skew settings
From: Mike Looijmans @ 2016-10-05 14:03 UTC (permalink / raw)
To: devicetree
Cc: netdev, linux-kernel, davem, robh+dt, mark.rutland, andrew,
f.fainelli, Mike Looijmans
The KSZ9031 skew registers contain an offset, the chip's default value
is "neutral" which does not add any skew. Programming a 0 into a skew
property will actually set it the maximal negative adjustment and not
to a neutral position as one would expect.
Explain this situation in the devicetree binding documentation and list
the settings that the chip considers neutral.
Changing the implementation to accept negative values would have been
a better solution, but would break existing configurations.
Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt b/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
index f9c32ad..c35b5b4 100644
--- a/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
+++ b/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
@@ -34,16 +34,17 @@ KSZ9031:
All skew control options are specified in picoseconds. The minimum
value is 0, and the maximum is property-dependent. The increment
- step is 60ps.
+ step is 60ps. The default value is the neutral setting, so setting
+ rxc-skew-ps=<0> actually results in -900 picoseconds adjustment.
Optional properties:
- Maximum value of 1860:
+ Maximum value of 1860, default value 900:
- rxc-skew-ps : Skew control of RX clock pad
- txc-skew-ps : Skew control of TX clock pad
- Maximum value of 900:
+ Maximum value of 900, default value 420:
- rxdv-skew-ps : Skew control of RX CTL pad
- txen-skew-ps : Skew control of TX CTL pad
--
1.9.1
^ permalink raw reply related
* Re: Intel Ethernet driver igb causes huge latencies with cyclictest (rt-tests)
From: Greg @ 2016-10-05 14:04 UTC (permalink / raw)
To: Koehrer Mathias (ETAS/ESW5); +Cc: netdev@vger.kernel.org
In-Reply-To: <d18971eee9ad4d13b8f59304063b932a@FE-MBX1012.de.bosch.com>
On Wed, 2016-10-05 at 08:29 +0000, Koehrer Mathias (ETAS/ESW5) wrote:
> Hi all,
>
> I noticed that with fairly new versions of the Linux kernel, the igb driver
> causes huge latencies with the cyclictest in a RT_PREEMPT environment.
> The root cause seems to be the number of interrupts that are used for the igb
> NIC devices as multiple of these irqs may occur at the same time (see below).
>
> With the kernel 4.6.7-rt14 the igb uses 9 (!) irqs per NIC on an Intel Core i7 PC (x86-64):
> E.g. eth2, and eth2-TxRx-0, eth2-TxRx-1, ... , eth2-TxRx-7.
>
> Running the very same machine with kernel 3.18.27-rt27 there are only 2 irqs:
> eth2 and eth2-TxRx0
>
> The issue with the many irqs is now that they are all fired roughly the same time
> even if the link is down as nothing is connected to the NIC.
> I analyzed the execution of the cyclictest tool using the kernel tracer on kernel 4.6.7-rt14:
>
> kworker/-5 0dN.h2.. 1504647372us : sched_wakeup: comm=cyclictest pid=5887 prio=19 target_cpu=000
> kworker/-5 0dN.h3.. 1504647374us : sched_wakeup: comm=irq/54-eth2-TxR pid=5883 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647375us : sched_wakeup: comm=irq/53-eth2-TxR pid=5882 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647377us : sched_wakeup: comm=irq/52-eth2-TxR pid=5881 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647378us : sched_wakeup: comm=irq/51-eth2-TxR pid=5880 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647380us : sched_wakeup: comm=irq/50-eth2-TxR pid=5879 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647381us : sched_wakeup: comm=irq/49-eth2-TxR pid=5878 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647382us : sched_wakeup: comm=irq/48-eth2-TxR pid=5877 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647383us : sched_wakeup: comm=irq/47-eth2-TxR pid=5876 prio=49 target_cpu=000
> kworker/-5 0d...2.. 1504647384us : sched_switch: prev_comm=kworker/0:0 prev_pid=5 prev_prio=120 prev_state=R+ ==> next_comm=cyclictest next_pid=5887 next_prio=19
>
> Here it can be clearly seen that eight irqs from the igb are coming in at the same time.
> This leads to a fairly long phase of running in irq mode which hurts the real time latency.
>
> In my setup I have no cable connected to the eth2,
> I do a
> # modprobe igb
> # ifconfig eth2 up 192.168.100.111
>
> I did multiple tests with analyzing and modifying the igb driver.
> The function "igb_watchdog_task" seems to be the root cause of the issue.
> Whenever I disable this function the cyclictest shows great results.
>
> There has been lengthy discussion on that topic on the rt-users mailing list:
> http://marc.info/?t=147454836600003&r=1&w=2
>
> My question is now:
> How can I either use only 1 irq per NIC using the igb driver or how can
> the driver be reorganized to let the watchdog task trigger the irqs alternately.
Have you tried the ethtool channel command to reduce the number of
queues/channels? I don't have an Intel part but I can do this with a
broadcom:
[root@galilei ~]# ethtool -l em1
Channel parameters for em1:
Pre-set maximums:
RX: 4
TX: 4
Other: 0
Combined: 0
Current hardware settings:
RX: 4
TX: 1
Other: 0
Combined: 0
[root@galilei ~]# grep em1 /proc/interrupts
38: 16 2 2 145912 IR-PCI-MSI-edge
em1-tx-0
39: 80893 29784 2 0 IR-PCI-MSI-edge
em1-rx-1
40: 76123 281434 1 0 IR-PCI-MSI-edge
em1-rx-2
41: 5 0 240184 0 IR-PCI-MSI-edge
em1-rx-3
42: 2 1 0 16132 IR-PCI-MSI-edge
em1-rx-4
[root@galilei ~]# ethtool -L em1 rx 2 tx 2
[root@galilei ~]# grep em1 /proc/interrupts
38: 2 0 0 0 IR-PCI-MSI-edge
em1-0
39: 54 2 0 0 IR-PCI-MSI-edge
em1-txrx-1
40: 71 0 1 0 IR-PCI-MSI-edge
em1-txrx-
Give it a try and see if it helps.
- Greg
>
> Thanks for any feedback
>
> Regards
>
> Mathias
^ permalink raw reply
* [PATCH net] packet: call fanout_release, while UNREGISTERING a netdev
From: Anoob Soman @ 2016-10-05 14:12 UTC (permalink / raw)
To: netdev; +Cc: Anoob Soman
If a socket has FANOUT sockopt set, a new proto_hook is registered
as part of fanout_add(). When processing a NETDEV_UNREGISTER event in
af_packet, __fanout_unlink is called for all sockets, but prot_hook which was
registered as part of fanout_add is not removed. Call fanout_release, on a
NETDEV_UNREGISTER, which removes prot_hook and removes fanout from the
fanout_list.
This fixes BUG_ON(!list_empty(&dev->ptype_specific)) in netdev_run_todo()
Signed-off-by: Anoob Soman <anoob.soman@citrix.com>
---
net/packet/af_packet.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 33a4697..11db0d6 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3952,6 +3952,7 @@ static int packet_notifier(struct notifier_block *this,
}
if (msg == NETDEV_UNREGISTER) {
packet_cached_dev_reset(po);
+ fanout_release(sk);
po->ifindex = -1;
if (po->prot_hook.dev)
dev_put(po->prot_hook.dev);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Eyal Birger @ 2016-10-05 14:18 UTC (permalink / raw)
To: Jiri Benc; +Cc: netdev@vger.kernel.org, pravin shelar, Eric Garver
In-Reply-To: <c7f44c08104958c86c68d98f1e093d754b0ca61b.1475672569.git.jbenc@redhat.com>
Hi,
On Wed, Oct 5, 2016 at 4:07 PM, Jiri Benc <jbenc@redhat.com> wrote:
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 4d67ea856067..c47b3da8ecf2 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -594,6 +594,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
> else
> packet->protocol = htons(ETH_P_802_2);
>
> + if (eth_type_vlan(packet->protocol)) {
> + __skb_pull(packet, ETH_HLEN);
> + skb_reset_network_header(packet);
> + skb_reset_mac_len(packet);
> + packet = skb_vlan_untag(packet);
> + if (unlikely(!packet))
> + goto err;
I think at this point, 'eth' may point to a freed packet.
Maybe replace the 'eth' variable with a 'proto' variable caching the
value of eth_hdr(packet)->h_proto?
> + skb_push(packet, ETH_HLEN);
> + }
> +
> /* Set packet's mru */
> if (a[OVS_PACKET_ATTR_MRU]) {
> mru = nla_get_u16(a[OVS_PACKET_ATTR_MRU]);
> --
> 1.8.3.1
>
Eyal.
^ permalink raw reply
* Re: [Xen-devel] [PATCH v2 net-next 2/7] xen-netback: retire guest rx side prefix GSO feature
From: Roger Pau Monné @ 2016-10-05 15:30 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Paul Durrant, bouyer, annie.li@oracle.com,
joao.m.martins@oracle.com, netdev@vger.kernel.org,
xen-devel@lists.xenproject.org, Wei Liu
In-Reply-To: <20161004142404.GC18821@char.us.oracle.com>
On Tue, Oct 04, 2016 at 10:24:04AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 04, 2016 at 01:35:41PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > Sent: 04 October 2016 13:52
> > > To: Paul Durrant <Paul.Durrant@citrix.com>; annie.li@oracle.com;
> > > joao.m.martins@oracle.com
> > > Cc: netdev@vger.kernel.org; xen-devel@lists.xenproject.org; Wei Liu
> > > <wei.liu2@citrix.com>
> > > Subject: Re: [Xen-devel] [PATCH v2 net-next 2/7] xen-netback: retire guest
> > > rx side prefix GSO feature
> > >
> > > On Tue, Oct 04, 2016 at 10:29:13AM +0100, Paul Durrant wrote:
> > > > As far as I am aware only very old Windows network frontends make use
> > > > of this style of passing GSO packets from backend to frontend. These
> > > > frontends can easily be replaced by the freely available Xen Project
> > > > Windows PV network frontend, which uses the 'default' mechanism for
> > > > passing GSO packets, which is also used by all Linux frontends.
> > >
> > > It is not that simple. Some companies have extra juice in their Windows
> > > frontends so can't easily swap over to the Xen Project one.
> >
> > Ok, then those frontends will continue to work, but they won't get GSO packets any more. Prefix GSO has never been specified in the canonical netif header and so has been in a limbo state forever so such frontends have always been on borrowed time and only just happened to work against a linux backend. If someone wants to actually specify prefix GSO properly then it could be added back in, but it should not be necessary now that the RX side req<->rsp identity relation is documented (http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/io/netif.h;hb=HEAD#l729).
> >
> > >
> > > Either way CC-ing Annie
> > >
> > > Also would it make sense to CC the FreeBSD and NetBSD maintainers of their
> > > PV drivers just to make sure? (Or has that been confirmed)
> > >
> >
> > I could do that, but I'd hope that they would be subscribed to xen-devel and will chime in if there's likely to be a problem.
>
> Usually one CCs those folks. I think you are asking me to do
> the legwork and find them and CC them here?
>
> CC-ing Roger and Manuel Bouyer.
Thanks. FreeBSD is using the same method as current Linux in order to both
send and receive GSO packets. That is using an extra slot in the ring,
filled with a netif_extra_info of type XEN_NETIF_EXTRA_TYPE_GSO. Full code
can be found here [0], but AFAICT FreeBSD is not using this prefix stuff.
Also, IIRC NetBSD doesn't have a Xen GSO implementation [1], but I would let
Manuel answer that one.
Roger.
[0] http://fxr.watson.org/fxr/source/dev/xen/netfront/netfront.c
[1] https://github.com/jsonn/src/blob/trunk/sys/arch/xen/xen/if_xennet_xenbus.c
^ permalink raw reply
* Re: [PATCH v2 net-next 2/7] xen-netback: retire guest rx side prefix GSO feature
From: Manuel Bouyer @ 2016-10-05 15:40 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Wei Liu, netdev@vger.kernel.org, annie.li@oracle.com,
Paul Durrant, xen-devel@lists.xenproject.org,
joao.m.martins@oracle.com
In-Reply-To: <20161005153026.ihxdtvu3hgpjql4o@mac>
On Wed, Oct 05, 2016 at 05:30:26PM +0200, Roger Pau Monné wrote:
> [...]
> Also, IIRC NetBSD doesn't have a Xen GSO implementation [1], but I would let
> Manuel answer that one.
I confirm, we don't support GSO at this time.
--
Manuel Bouyer <bouyer@antioche.eu.org>
NetBSD: 26 ans d'experience feront toujours la difference
--
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply
* Re: Kernel 4.6.7-rt13: Intel Ethernet driver igb causes huge latencies in cyclictest
From: Julia Cartwright @ 2016-10-05 15:59 UTC (permalink / raw)
To: Koehrer Mathias (ETAS/ESW5), Jeff Kirsher
Cc: Sebastian Andrzej Siewior, linux-rt-users@vger.kernel.org,
intel-wired-lan, netdev
In-Reply-To: <584755c2766e4b94a604ece16760fe14@FE-MBX1012.de.bosch.com>
On Wed, Oct 05, 2016 at 07:02:21AM +0000, Koehrer Mathias (ETAS/ESW5) wrote:
> Hi Julia,
>
> > > In the meanwhile I have detected another finding which might be relevant:
> > >
> > > With the 3.18 kernel the igb driver comes with two interrupts per
> > > NIC (e.g. eth2 and eth2-TxRx0) with the 4.6. kernel the igb driver
> > > comes with 9 (!) interrupts per NIC: eth2, and eth2-TxRx-0,
> > > eth2-TxRx-1, ... , eth2-TxRx-7.
> > >
> > > As I have used initially the same kernel configuration from 3.18 also
> > > for the 4.6. kernel I wonder where this comes from and if there is any
> > > kernel option I may use to disable these many interrupts and to reduce
> > > it to 2 again.
> >
> > If it's all of these interrupts that are firing and being handled at the same time, that
> > can account for the latencies you were seeing. As I suggested before, having a
> > trace with the sched_wakeup event enabled can help confirm that it's these
> > interrupts causing problems.
> >
> > If it is true, then the question is: why is the device triggering all of these interrupts all
> > at once? Is that expected? These are questions for netdev folks, I think.
> >
> > Julia
>
> OK - I ran again the cyclictest. This time I used the -C option:
> # cyclictest -a -i 100 -m -n -p 80 -t 1 -b 21 -C
>
> And the last output lines of the trace are:
> cyclicte-5887 0d...2.. 1504647266us!: sched_switch: prev_comm=cyclictest prev_pid=5887 prev_prio=19 prev_state=S ==> next_comm=kworker/0:0 next_pid=5 next_prio=120
> kworker/-5 0dN.h2.. 1504647372us : sched_wakeup: comm=cyclictest pid=5887 prio=19 target_cpu=000
> kworker/-5 0dN.h3.. 1504647374us : sched_wakeup: comm=irq/54-eth2-TxR pid=5883 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647375us : sched_wakeup: comm=irq/53-eth2-TxR pid=5882 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647377us : sched_wakeup: comm=irq/52-eth2-TxR pid=5881 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647378us : sched_wakeup: comm=irq/51-eth2-TxR pid=5880 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647380us : sched_wakeup: comm=irq/50-eth2-TxR pid=5879 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647381us : sched_wakeup: comm=irq/49-eth2-TxR pid=5878 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647382us : sched_wakeup: comm=irq/48-eth2-TxR pid=5877 prio=49 target_cpu=000
> kworker/-5 0dN.h3.. 1504647383us : sched_wakeup: comm=irq/47-eth2-TxR pid=5876 prio=49 target_cpu=000
> kworker/-5 0d...2.. 1504647384us : sched_switch: prev_comm=kworker/0:0 prev_pid=5 prev_prio=120 prev_state=R+ ==> next_comm=cyclictest next_pid=5887 next_prio=19
> cyclicte-5887 0.....11 1504647389us : tracing_mark_write: hit latency threshold (28 > 21)
>
> The attached trace-extract.gz shows some more lines.
> It actually looks to me as if the the many irq threads from igb are causing the issue.
Yes, I think so.
Although, to be clear, it isn't the fact that there exists 8 threads,
it's that the device is firing all 8 interrupts at the same time. The
time spent in hardirq context just waking up all 8 of those threads (and
the cyclictest wakeup) is enough to cause your regression.
netdev/igb folks-
Under what conditions should it be expected that the i350 trigger all of
the TxRx interrupts simultaneously? Any ideas here?
See the start of this thread here:
http://lkml.kernel.org/r/d648628329bc446fa63b5e19d4d3fb56@FE-MBX1012.de.bosch.com
Julia
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Jiri Benc @ 2016-10-05 17:23 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev@vger.kernel.org, pravin shelar, Eric Garver
In-Reply-To: <CAHsH6Gvha_Saq8SVaLhsXzNggAOwbdu6n5xLQD7HFhYweJMsmw@mail.gmail.com>
On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
> I think at this point, 'eth' may point to a freed packet.
It may but how does that matter? eth is not used beyond that point.
Jiri
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Eyal Birger @ 2016-10-05 17:31 UTC (permalink / raw)
To: Jiri Benc; +Cc: netdev@vger.kernel.org, pravin shelar, Eric Garver
In-Reply-To: <20161005192319.713d92e1@griffin>
On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote:
> On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
>> I think at this point, 'eth' may point to a freed packet.
>
> It may but how does that matter? eth is not used beyond that point.
Definitely a nit. For sure not critical.
Just seemed less future safe to keep a pointer to an old packet lying around.
Eyal.
^ permalink raw reply
* Re: [PATCH] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Joe Perches @ 2016-10-05 17:53 UTC (permalink / raw)
To: Marcel Holtmann, Pavel Machek
Cc: trivial, Gustavo F. Padovan, Johan Hedberg, David S. Miller,
linux-bluetooth, netdev, linux-kernel
In-Reply-To: <368810F9-519C-44B5-85D4-1815273438BF@holtmann.org>
On Wed, 2016-10-05 at 13:14 +0200, Marcel Holtmann wrote:
> Hi Pavel,
>
> > bluetooth.h is not part of user API, so __ variants are not neccessary
> > here.
> >
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
[]
> > struct bt_skb_cb {
> > - __u8 pkt_type;
> > - __u8 force_active;
> > - __u16 expect;
> > - __u8 incoming:1;
> > + u8 pkt_type;
> > + u8 force_active;
> > + u16 expect;
> > + u8 incoming:1;
> > union {
> > struct l2cap_ctrl l2cap;
> > struct hci_ctrl hci;
trivia:
It's generally faster to use bool instead of u8 foo:1;
^ permalink raw reply
* Re: [PATCH net] Panic when tc_lookup_action_n finds a partially initialized action.
From: Cong Wang @ 2016-10-05 18:01 UTC (permalink / raw)
To: Krister Johansen; +Cc: Jamal Hadi Salim, Linux Kernel Network Developers
In-Reply-To: <20161005065244.GA2245@templeofstupid.com>
[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
On Tue, Oct 4, 2016 at 11:52 PM, Krister Johansen
<kjlx@templeofstupid.com> wrote:
> On Mon, Oct 03, 2016 at 11:22:33AM -0700, Cong Wang wrote:
>> Please try the attached patch. I also convert the read path to RCU
>> to avoid a possible deadlock. A quick test shows no lockdep splat.
>
> I tried this patch, but it doesn't solve the problem. I got a panic on
> my very first try:
Thanks for testing it.
> The problem here is the same as before: by using RCU the race isn't
> fixed because the module is still discoverable from act_base before the
> pernet initialization is completed.
>
> You can see from the trap frame that the first two arguments to
> tcf_hash_check were 0. It couldn't look up the correct per-subsystem
> pointer because the id hadn't yet been registered.
I thought the problem is that we don't do pernet ops registration and
action ops registration atomically therefore chose to use mutex+RCU,
but I was wrong, the problem here is just ordering, we need to finish
the pernet initialization before making action ops visible.
If so, why not just reorder them? Does the attached patch make any
sense now? Our pernet init doesn't rely on act_base, so even we have
some race, the worst case is after we initialize the pernet netns for an
action but its ops still not visible, which seems fine (at least no crash).
Or I still miss something here?
(Sorry that I don't have the environment to reproduce your bug)
Thanks for your patience and testing!
[-- Attachment #2: act_api_register.diff --]
[-- Type: text/plain, Size: 1306 bytes --]
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d09d068..6024920 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -341,22 +341,21 @@ int tcf_register_action(struct tc_action_ops *act,
if (!act->act || !act->dump || !act->init || !act->walk || !act->lookup)
return -EINVAL;
+ ret = register_pernet_subsys(ops);
+ if (ret)
+ return ret;
+
write_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
write_unlock(&act_mod_lock);
+ unregister_pernet_subsys(ops);
return -EEXIST;
}
}
list_add_tail(&act->head, &act_base);
write_unlock(&act_mod_lock);
- ret = register_pernet_subsys(ops);
- if (ret) {
- tcf_unregister_action(act, ops);
- return ret;
- }
-
return 0;
}
EXPORT_SYMBOL(tcf_register_action);
@@ -367,8 +366,6 @@ int tcf_unregister_action(struct tc_action_ops *act,
struct tc_action_ops *a;
int err = -ENOENT;
- unregister_pernet_subsys(ops);
-
write_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (a == act) {
@@ -378,6 +375,8 @@ int tcf_unregister_action(struct tc_action_ops *act,
}
}
write_unlock(&act_mod_lock);
+ if (!err)
+ unregister_pernet_subsys(ops);
return err;
}
EXPORT_SYMBOL(tcf_unregister_action);
^ permalink raw reply related
* Re: [PATCH net] Panic when tc_lookup_action_n finds a partially initialized action.
From: Cong Wang @ 2016-10-05 18:07 UTC (permalink / raw)
To: Krister Johansen; +Cc: Jamal Hadi Salim, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpUya1ORUUNC9-spLKHjsjNyj6xHO79kvoDGaZcWT_mEwg@mail.gmail.com>
On Wed, Oct 5, 2016 at 11:01 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Oct 4, 2016 at 11:52 PM, Krister Johansen
> <kjlx@templeofstupid.com> wrote:
>> On Mon, Oct 03, 2016 at 11:22:33AM -0700, Cong Wang wrote:
>>> Please try the attached patch. I also convert the read path to RCU
>>> to avoid a possible deadlock. A quick test shows no lockdep splat.
>>
>> I tried this patch, but it doesn't solve the problem. I got a panic on
>> my very first try:
>
> Thanks for testing it.
>
>
>> The problem here is the same as before: by using RCU the race isn't
>> fixed because the module is still discoverable from act_base before the
>> pernet initialization is completed.
>>
>> You can see from the trap frame that the first two arguments to
>> tcf_hash_check were 0. It couldn't look up the correct per-subsystem
>> pointer because the id hadn't yet been registered.
>
> I thought the problem is that we don't do pernet ops registration and
> action ops registration atomically therefore chose to use mutex+RCU,
> but I was wrong, the problem here is just ordering, we need to finish
> the pernet initialization before making action ops visible.
>
> If so, why not just reorder them? Does the attached patch make any
> sense now? Our pernet init doesn't rely on act_base, so even we have
> some race, the worst case is after we initialize the pernet netns for an
> action but its ops still not visible, which seems fine (at least no crash).
BTW, I should remove the 'if' check for unregister_pernet_subsys() in
tcf_unregister_action()... Otherwise the error path doesn't work. ;-)
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Eric Garver @ 2016-10-05 18:44 UTC (permalink / raw)
To: Eyal Birger; +Cc: Jiri Benc, netdev@vger.kernel.org, pravin shelar
In-Reply-To: <CAHsH6GvO=F38jXo18=3n1+w+sojRtRcLgbdE9PrGbGyB4vmoEA@mail.gmail.com>
On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote:
> > On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
> >> I think at this point, 'eth' may point to a freed packet.
> >
> > It may but how does that matter? eth is not used beyond that point.
>
> Definitely a nit. For sure not critical.
>
> Just seemed less future safe to keep a pointer to an old packet lying around.
I agree. Alternatively refresh the eth pointer.
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Jiri Benc @ 2016-10-05 19:07 UTC (permalink / raw)
To: Eric Garver; +Cc: Eyal Birger, netdev@vger.kernel.org, pravin shelar
In-Reply-To: <20161005184426.GQ25403@egarver>
On Wed, 5 Oct 2016 14:44:26 -0400, Eric Garver wrote:
> On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> > Just seemed less future safe to keep a pointer to an old packet lying around.
>
> I agree. Alternatively refresh the eth pointer.
Sorry guys, that just doesn't make sense. Everyone should know that
reloading of skb pointer means the former pointers to its data may
become invalid. Please point me to any place in the kernel where we
reload the data pointer "just because" even when not used.
Jiri
^ permalink raw reply
* Re: [PATCH] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Pavel Machek @ 2016-10-05 19:11 UTC (permalink / raw)
To: Joe Perches
Cc: Marcel Holtmann, trivial-DgEjT+Ai2ygdnm+yROfE0A,
Gustavo F. Padovan, Johan Hedberg, David S. Miller,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1475689996.6105.11.camel-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]
On Wed 2016-10-05 10:53:16, Joe Perches wrote:
> On Wed, 2016-10-05 at 13:14 +0200, Marcel Holtmann wrote:
> > Hi Pavel,
> >
> > > bluetooth.h is not part of user API, so __ variants are not neccessary
> > > here.
> > >
> > > Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > >
> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> []
> > > struct bt_skb_cb {
> > > - __u8 pkt_type;
> > > - __u8 force_active;
> > > - __u16 expect;
> > > - __u8 incoming:1;
> > > + u8 pkt_type;
> > > + u8 force_active;
> > > + u16 expect;
> > > + u8 incoming:1;
> > > union {
> > > struct l2cap_ctrl l2cap;
> > > struct hci_ctrl hci;
>
> trivia:
>
> It's generally faster to use bool instead of u8 foo:1;
Ok, but I'm not changing that in this patch.
(And actually, bool will take a lot more memory, right?)
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Eric Dumazet @ 2016-10-05 19:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Alexei Starovoitov, Greg Thelen
From: Eric Dumazet <edumazet@google.com>
Since linux-3.15, netlink_dump() can use up to 16384 bytes skb
allocations.
Due to struct skb_shared_info ~320 bytes overhead, we end up using
order-3 (on x86) page allocations, that might trigger direct reclaim and
add stress.
The intent was really to attempt a large allocation but immediately
fallback to a smaller one (order-1 on x86) in case of memory stress.
On recent kernels (linux-4.4), we can remove __GFP_DIRECT_RECLAIM to
meet the goal. Old kernels would need to remove __GFP_WAIT
While we are at it, since we do an order-3 allocation, allow to use
all the allocated bytes instead of 16384 to reduce syscalls during
large dumps.
iproute2 already uses 32KB recvmsg() buffer sizes.
Alexei provided an initial patch downsizing to SKB_WITH_OVERHEAD(16384)
Fixes: 9063e21fb026 ("netlink: autosize skb lengthes")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Alexei Starovoitov <ast@kernel.org>
Cc: Greg Thelen <gthelen@google.com>
---
Note: This will apply to net tree when it has synced with Linus tree.
net/netlink/af_netlink.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
/* Record the max length of recvmsg() calls for future allocations */
nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
- 16384);
+ SKB_WITH_OVERHEAD(32768));
copied = data_skb->len;
if (len < copied) {
@@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
if (alloc_min_size < nlk->max_recvmsg_len) {
alloc_size = nlk->max_recvmsg_len;
- skb = alloc_skb(alloc_size, GFP_KERNEL |
- __GFP_NOWARN | __GFP_NORETRY);
+ skb = alloc_skb(alloc_size,
+ (GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
+ __GFP_NOWARN | __GFP_NORETRY);
}
if (!skb) {
alloc_size = alloc_min_size;
^ permalink raw reply related
* Re: [PATCH] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Joe Perches @ 2016-10-05 19:15 UTC (permalink / raw)
To: Pavel Machek
Cc: Marcel Holtmann, trivial, Gustavo F. Padovan, Johan Hedberg,
David S. Miller, linux-bluetooth, netdev, linux-kernel
In-Reply-To: <20161005191103.GA25972@amd>
On Wed, 2016-10-05 at 21:11 +0200, Pavel Machek wrote:
> On Wed 2016-10-05 10:53:16, Joe Perches wrote:
> > On Wed, 2016-10-05 at 13:14 +0200, Marcel Holtmann wrote:
> > > Hi Pavel,
> > >
> > > > bluetooth.h is not part of user API, so __ variants are not neccessary
> > > > here.
> > > >
> > > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > >
> > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >
> > []
> > > > struct bt_skb_cb {
> > > > - __u8 pkt_type;
> > > > - __u8 force_active;
> > > > - __u16 expect;
> > > > - __u8 incoming:1;
> > > > + u8 pkt_type;
> > > > + u8 force_active;
> > > > + u16 expect;
> > > > + u8 incoming:1;
> > > > union {
> > > > struct l2cap_ctrl l2cap;
> > > > struct hci_ctrl hci;
> >
> >
> > trivia:
> >
> > It's generally faster to use bool instead of u8 foo:1;
>
> Ok, but I'm not changing that in this patch.
> (And actually, bool will take a lot more memory, right?)
No worries, and bool is the same size as u8.
>
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Eric Garver @ 2016-10-05 19:21 UTC (permalink / raw)
To: Jiri Benc; +Cc: Eyal Birger, netdev@vger.kernel.org, pravin shelar
In-Reply-To: <20161005210709.79732b27@griffin>
On Wed, Oct 05, 2016 at 09:07:09PM +0200, Jiri Benc wrote:
> On Wed, 5 Oct 2016 14:44:26 -0400, Eric Garver wrote:
> > On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> > > Just seemed less future safe to keep a pointer to an old packet lying around.
> >
> > I agree. Alternatively refresh the eth pointer.
>
> Sorry guys, that just doesn't make sense. Everyone should know that
> reloading of skb pointer means the former pointers to its data may
> become invalid. Please point me to any place in the kernel where we
> reload the data pointer "just because" even when not used.
>
How about this incremental change?
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 7ef02752d4ba..0dd36f353c53 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -562,7 +562,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
struct sw_flow *flow;
struct sw_flow_actions *sf_acts;
struct datapath *dp;
- struct ethhdr *eth;
struct vport *input_vport;
u16 mru = 0;
int len;
@@ -584,14 +583,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
nla_memcpy(__skb_put(packet, len), a[OVS_PACKET_ATTR_PACKET], len);
skb_reset_mac_header(packet);
- eth = eth_hdr(packet);
/* Normally, setting the skb 'protocol' field would be handled by a
* call to eth_type_trans(), but it assumes there's a sending
* device, which we may not have. */
- if (eth_proto_is_802_3(eth->h_proto))
- packet->protocol = eth->h_proto;
- else
+ packet->protocol = eth_hdr(packet)->h_proto;
+ if (!eth_proto_is_802_3(packet->protocol))
packet->protocol = htons(ETH_P_802_2);
if (eth_type_vlan(packet->protocol)) {
^ permalink raw reply related
* [PATCH] net: bgmac: Fix errant feature flag check
From: Jon Mason @ 2016-10-05 19:36 UTC (permalink / raw)
To: davem; +Cc: f.fainelli, zajec5, bcm-kernel-feedback-list, netdev,
linux-kernel
During the conversion to the feature flags, a check against
ci->id != BCMA_CHIP_ID_BCM47162
became
bgmac->feature_flags & BGMAC_FEAT_CLKCTLS
instead of
!(bgmac->feature_flags & BGMAC_FEAT_CLKCTLS)
Reported-by: Rafał Miłecki <zajec5@gmail.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index c4751ec..87d00ea 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1024,7 +1024,7 @@ static void bgmac_enable(struct bgmac *bgmac)
mode = (bgmac_read(bgmac, BGMAC_DEV_STATUS) & BGMAC_DS_MM_MASK) >>
BGMAC_DS_MM_SHIFT;
- if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST || mode != 0)
+ if (!(bgmac->feature_flags & BGMAC_FEAT_CLKCTLST) || mode != 0)
bgmac_set(bgmac, BCMA_CLKCTLST, BCMA_CLKCTLST_FORCEHT);
if (bgmac->feature_flags & BGMAC_FEAT_CLKCTLST && mode == 2)
bgmac_cco_ctl_maskset(bgmac, 1, ~0,
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net] Fixing a bug in team driver due to incorrect 'unsigned int' to 'int' conversion
From: Eric Dumazet @ 2016-10-05 19:37 UTC (permalink / raw)
To: Alex Sidorenko; +Cc: netdev
In-Reply-To: <2440068.EFRMUI8i5V@zbook>
On Wed, 2016-10-05 at 09:06 -0400, Alex Sidorenko wrote:
> Roundrobin runner of team driver uses 'unsigned int' variable to count the number of sent_packets.
> Later it is passed to a subroutine team_num_to_port_index(struct team *team, int num) as
> 'num' and when we reach MAXINT (2**31-1), 'num' becomes negative.
>
> This leads to using incorrect hash-bucket for port lookup and as a result, packets are dropped. The fix
> consists of changing 'int num' to 'unsigned int num'. Testing of a fixed kernel shows that there
> is no packet drop anymore.
>
>
> Signed-off-by: Alex Sidorenko <alexandre.sidorenko@hpe.com>
Note that lines in your changelog are longer than the norm
( Documentation/SubmittingPatches around line 619 )
- The body of the explanation, line wrapped at 75 columns, which will
be copied to the permanent changelog to describe this patch.
Otherwise, patch looks, welcome to the club Alex !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Greg @ 2016-10-05 19:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen
In-Reply-To: <1475694798.28155.215.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, 2016-10-06 at 04:13 +0900, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Since linux-3.15, netlink_dump() can use up to 16384 bytes skb
> allocations.
>
> Due to struct skb_shared_info ~320 bytes overhead, we end up using
> order-3 (on x86) page allocations, that might trigger direct reclaim and
> add stress.
>
> The intent was really to attempt a large allocation but immediately
> fallback to a smaller one (order-1 on x86) in case of memory stress.
>
> On recent kernels (linux-4.4), we can remove __GFP_DIRECT_RECLAIM to
> meet the goal. Old kernels would need to remove __GFP_WAIT
>
> While we are at it, since we do an order-3 allocation, allow to use
> all the allocated bytes instead of 16384 to reduce syscalls during
> large dumps.
>
> iproute2 already uses 32KB recvmsg() buffer sizes.
>
> Alexei provided an initial patch downsizing to SKB_WITH_OVERHEAD(16384)
>
> Fixes: 9063e21fb026 ("netlink: autosize skb lengthes")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Cc: Greg Thelen <gthelen@google.com>
> ---
> Note: This will apply to net tree when it has synced with Linus tree.
>
> net/netlink/af_netlink.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> /* Record the max length of recvmsg() calls for future allocations */
> nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> - 16384);
> + SKB_WITH_OVERHEAD(32768));
>
> copied = data_skb->len;
> if (len < copied) {
> @@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
>
> if (alloc_min_size < nlk->max_recvmsg_len) {
> alloc_size = nlk->max_recvmsg_len;
> - skb = alloc_skb(alloc_size, GFP_KERNEL |
> - __GFP_NOWARN | __GFP_NORETRY);
> + skb = alloc_skb(alloc_size,
> + (GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
> + __GFP_NOWARN | __GFP_NORETRY);
> }
> if (!skb) {
> alloc_size = alloc_min_size;
>
>
This code has changed a lot since I first added it in 2011 but this
appears to be the right thing to do. I guess the order of operations
for the bitwise '&' and the bitwise '~' are correct, I don't have my C
manual laying around.
Reviewed-by: Greg Rose <grose@lightfleet.com>
^ permalink raw reply
* Re: [RFC v2 00/10] Landlock LSM: Unprivileged sandboxing
From: Mickaël Salaün @ 2016-10-05 20:30 UTC (permalink / raw)
To: Kees Cook
Cc: Pavel Machek, LKML, Alexei Starovoitov, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Daniel Borkmann, Daniel Mack,
David Drysdale, David S . Miller, Elena Reshetova, James Morris,
Paul Moore, Sargun Dhillon, Serge E . Hallyn, Will Drewry,
kernel-hardening@lists.openwall.com, Linux API,
linux-security-module, Network
In-Reply-To: <CAGXu5jLp=GMpjqdCCPWeKHNseEv5DLPC7YN6EwhR1PJsei5cHw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1931 bytes --]
On 04/10/2016 00:56, Kees Cook wrote:
> On Tue, Sep 20, 2016 at 10:08 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 15/09/2016 11:19, Pavel Machek wrote:
>>> Hi!
>>>
>>>> This series is a proof of concept to fill some missing part of seccomp as the
>>>> ability to check syscall argument pointers or creating more dynamic security
>>>> policies. The goal of this new stackable Linux Security Module (LSM) called
>>>> Landlock is to allow any process, including unprivileged ones, to create
>>>> powerful security sandboxes comparable to the Seatbelt/XNU Sandbox or the
>>>> OpenBSD Pledge. This kind of sandbox help to mitigate the security impact of
>>>> bugs or unexpected/malicious behaviors in userland applications.
>>>>
>>>> The first RFC [1] was focused on extending seccomp while staying at the syscall
>>>> level. This brought a working PoC but with some (mitigated) ToCToU race
>>>> conditions due to the seccomp ptrace hole (now fixed) and the non-atomic
>>>> syscall argument evaluation (hence the LSM hooks).
>>>
>>> Long and nice description follows. Should it go to Documentation/
>>> somewhere?
>>>
>>> Because some documentation would be useful...
>>> Pavel
>>
>> Right, but I was looking for feedback before investing in documentation. :)
>
> Heh, understood. There are a number of grammar issues that slow me
> down when reading this, so when it does move into Documentation/, I'll
> have some English nit-picks. :)
>
> While reading I found myself wanting an explicit list of "guiding
> principles" for anyone implementing new hooks. It is touched on in
> several places (don't expose things, don't allow for privilege
> changes, etc). Having that spelled out somewhere would be nice.
Right, I'm going to try to create a more consistent documentation with
the "guiding principles".
Mickaël
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* [PATCHv2] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Pavel Machek @ 2016-10-05 20:51 UTC (permalink / raw)
To: trivial, marcel, gustavo, johan.hedberg, davem, linux-bluetooth,
netdev, linux-kernel
In-Reply-To: <20161003085640.GA8130@amd>
[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]
bluetooth.h is not part of user API, so __ variants are not neccessary
here.
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
v2: not touching stuff that Marcel does not want touched, as it will
become API later.
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index bfd1590..aea0371 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -196,7 +196,7 @@ typedef struct {
#define BDADDR_LE_PUBLIC 0x01
#define BDADDR_LE_RANDOM 0x02
-static inline bool bdaddr_type_is_valid(__u8 type)
+static inline bool bdaddr_type_is_valid(u8 type)
{
switch (type) {
case BDADDR_BREDR:
@@ -208,7 +208,7 @@ static inline bool bdaddr_type_is_valid(__u8 type)
return false;
}
-static inline bool bdaddr_type_is_le(__u8 type)
+static inline bool bdaddr_type_is_le(u8 type)
{
switch (type) {
case BDADDR_LE_PUBLIC:
@@ -278,15 +278,16 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
/* Skb helpers */
struct l2cap_ctrl {
- __u8 sframe:1,
+ u8 sframe:1,
poll:1,
final:1,
fcs:1,
sar:2,
super:2;
- __u16 reqseq;
- __u16 txseq;
- __u8 retries;
+
+ u16 reqseq;
+ u16 txseq;
+ u8 retries;
__le16 psm;
bdaddr_t bdaddr;
struct l2cap_chan *chan;
@@ -302,7 +303,7 @@ typedef void (*hci_req_complete_skb_t)(struct hci_dev *hdev, u8 status,
#define HCI_REQ_SKB BIT(1)
struct hci_ctrl {
- __u16 opcode;
+ u16 opcode;
u8 req_flags;
u8 req_event;
union {
@@ -312,10 +313,10 @@ struct hci_ctrl {
};
struct bt_skb_cb {
- __u8 pkt_type;
- __u8 force_active;
- __u16 expect;
- __u8 incoming:1;
+ u8 pkt_type;
+ u8 force_active;
+ u16 expect;
+ u8 incoming:1;
union {
struct l2cap_ctrl l2cap;
struct hci_ctrl hci;
@@ -365,7 +366,7 @@ out:
return NULL;
}
-int bt_to_errno(__u16 code);
+int bt_to_errno(u16 code);
void hci_sock_set_flag(struct sock *sk, int nr);
void hci_sock_clear_flag(struct sock *sk, int nr);
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply related
* error: 'struct net_device' has no member named 'nf_hooks_ingress'
From: Michal Sojka @ 2016-10-05 20:56 UTC (permalink / raw)
To: Sergey Senozhatsky, Aaron Conole
Cc: linux-next, linux-kernel, Stephen Rothwell, Florian Westphal,
Pablo Neira Ayuso, netdev, netfilter-devel, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20161004004801.GA4661@swordfish>
Hi,
On Tue, Oct 04 2016, Sergey Senozhatsky wrote:
> On (09/27/16 19:03), Sergey Senozhatsky wrote:
>> Hello,
>>
>> On (09/27/16 16:40), Stephen Rothwell wrote:
>> >
>> > Changes since 20160923:
>> >
>>
>> seems that commit e3b37f11e6e4e6b6 ("netfilter: replace list_head with
>> single linked list") breaks the build on !CONFIG_NETFILTER_INGRESS systems
>> accessing ->nf_hooks_ingress
this commit is now in mainline as
e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d and it breaks my build:
net/netfilter/core.c: In function 'nf_set_hooks_head':
net/netfilter/core.c:96:3: error: 'struct net_device' has no member named 'nf_hooks_ingress'
Are the fixes (see below) on the way to mainline too?
Thanks.
-Michal
>>
>> static void nf_set_hooks_head(struct net *net, const struct nf_hook_ops *reg,
>> struct nf_hook_entry *entry)
>> {
>> switch (reg->pf) {
>> case NFPROTO_NETDEV:
>> /* We already checked in nf_register_net_hook() that this is
>> * used from ingress.
>> */
>> rcu_assign_pointer(reg->dev->nf_hooks_ingress, entry);
>> ^^^^^^^^^^^^^^^^^^^^
>
>
> so I see two commits in linux-next now that fix the commit in question in
> two patches
>
> : commit 7816ec564ec40ae20bb7925f733a181cad0cc491 ("netfilter: accommodate
> : different kconfig in nf_set_hooks_head")
> :
> : When CONFIG_NETFILTER_INGRESS is unset (or no), we need to handle
> : the request for registration properly by dropping the hook. This
> : releases the entry during the set.
> :
> : Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
>
> and
>
> : commit 5119e4381a90fabd3442bde02707cbd9e5d7367a ("netfilter: Fix potential
> : null pointer dereference")
> :
> : It's possible for nf_hook_entry_head to return NULL. If two
> : nf_unregister_net_hook calls happen simultaneously with a single hook
> : entry in the list, both will enter the nf_hook_mutex critical section.
> : The first will successfully delete the head, but the second will see
> : this NULL pointer and attempt to dereference.
> :
> : This fix ensures that no null pointer dereference could occur when such
> : a condition happens.
> :
> : Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
>
>
> do you guys plan to fold those into "e3b37f11e6e4" (a preferred way)
> or will send it out as 3 separate patches (um, why) ?
>
> -ss
^ permalink raw reply
* Re: [RFC v3 16/22] bpf/cgroup,landlock: Handle Landlock hooks per cgroup
From: Mickaël Salaün @ 2016-10-05 20:58 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Daniel Borkmann, Daniel Mack, David Drysdale,
David S . Miller, Elena Reshetova, Eric W . Biederman,
James Morris, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Tejun Heo, Will Drewry, kernel-hardening@lists.openwall.com,
Linux API, linux-security-module <linux-s
In-Reply-To: <CAGXu5jJ-Heb0nnLygYo0xHaJKSrQM5RSVRNYN2NnYddwGHtWkA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3826 bytes --]
On 04/10/2016 01:43, Kees Cook wrote:
> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> This allows to add new eBPF programs to Landlock hooks dedicated to a
>> cgroup thanks to the BPF_PROG_ATTACH command. Like for socket eBPF
>> programs, the Landlock hooks attached to a cgroup are propagated to the
>> nested cgroups. However, when a new Landlock program is attached to one
>> of this nested cgroup, this cgroup hierarchy fork the Landlock hooks.
>> This design is simple and match the current CONFIG_BPF_CGROUP
>> inheritance. The difference lie in the fact that Landlock programs can
>> only be stacked but not removed. This match the append-only seccomp
>> behavior. Userland is free to handle Landlock hooks attached to a cgroup
>> in more complicated ways (e.g. continuous inheritance), but care should
>> be taken to properly handle error cases (e.g. memory allocation errors).
>>
>> Changes since v2:
>> * new design based on BPF_PROG_ATTACH (suggested by Alexei Starovoitov)
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Daniel Mack <daniel@zonque.org>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Link: https://lkml.kernel.org/r/20160826021432.GA8291@ast-mbp.thefacebook.com
>> Link: https://lkml.kernel.org/r/20160827204307.GA43714@ast-mbp.thefacebook.com
>> ---
>> include/linux/bpf-cgroup.h | 7 +++++++
>> include/linux/cgroup-defs.h | 2 ++
>> include/linux/landlock.h | 9 +++++++++
>> include/uapi/linux/bpf.h | 1 +
>> kernel/bpf/cgroup.c | 33 ++++++++++++++++++++++++++++++---
>> kernel/bpf/syscall.c | 11 +++++++++++
>> security/landlock/lsm.c | 40 +++++++++++++++++++++++++++++++++++++++-
>> security/landlock/manager.c | 32 ++++++++++++++++++++++++++++++++
>> 8 files changed, 131 insertions(+), 4 deletions(-)
>>
>> [...]
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 7b75fa692617..1c18fe46958a 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -15,6 +15,7 @@
>> #include <linux/bpf.h>
>> #include <linux/bpf-cgroup.h>
>> #include <net/sock.h>
>> +#include <linux/landlock.h>
>>
>> DEFINE_STATIC_KEY_FALSE(cgroup_bpf_enabled_key);
>> EXPORT_SYMBOL(cgroup_bpf_enabled_key);
>> @@ -31,7 +32,15 @@ void cgroup_bpf_put(struct cgroup *cgrp)
>> union bpf_object pinned = cgrp->bpf.pinned[type];
>>
>> if (pinned.prog) {
>> - bpf_prog_put(pinned.prog);
>> + switch (type) {
>> + case BPF_CGROUP_LANDLOCK:
>> +#ifdef CONFIG_SECURITY_LANDLOCK
>> + put_landlock_hooks(pinned.hooks);
>> + break;
>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>> + default:
>> + bpf_prog_put(pinned.prog);
>> + }
>> static_branch_dec(&cgroup_bpf_enabled_key);
>> }
>> }
>
> I get creeped out by type-controlled unions of pointers. :P I don't
> have a suggestion to improve this, but I don't like seeing a pointer
> type managed separately from the pointer itself as it tends to bypass
> a lot of both static and dynamic checking. A union is better than a
> cast of void *, but it still worries me. :)
This is not fully satisfactory for me neither but the other approach is
to use two distinct struct fields instead of a union.
Do you prefer if there is a "type" field in the "pinned" struct to
select the union?
Mickaël
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox