* Re: [PATCH 2/3] module: Fix up module_notifier return values.
From: Frank Ch. Eigler @ 2019-06-24 20:58 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Jessica Yu, linux-kernel, Josh Poimboeuf, jikos,
mbenes, Petr Mladek, Alexei Starovoitov, Daniel Borkmann,
Andrew Morton, Robert Richter, rostedt, Ingo Molnar,
Martin KaFai Lau, Song Liu, Yonghong Song, paulmck,
Joel Fernandes, Google, Ard Biesheuvel, Thomas Gleixner,
oprofile-list, netdev, bpf
In-Reply-To: <320564860.243.1561384864186.JavaMail.zimbra@efficios.com>
Hi -
> > While auditing all module notifiers I noticed a whole bunch of fail
> > wrt the return value. Notifiers have a 'special' return semantics.
From peterz's comments, the patches, it's not obvious to me how one is
to choose between 0 (NOTIFY_DONE) and 1 (NOTIFY_OK) in the case of a
routine success.
> [...]
> I have a similar erroneous module notifier return value pattern
> in lttng-modules as well. I'll go fix it right away. CCing
> Frank Eigler from SystemTAP which AFAIK use a copy of
> lttng-tracepoint.c in their project, which should be fixed
> as well. I'm pasting the lttng-modules fix below.
Sure, following suit. Thanks.
- FChE
^ permalink raw reply
* Re: WWAN Controller Framework (was IPA [PATCH v2 00/17])
From: Alex Elder @ 2019-06-24 21:16 UTC (permalink / raw)
To: Dan Williams, davem, arnd, bjorn.andersson, ilias.apalodimas
Cc: evgreen, benchan, ejcaruso, cpratapa, syadagir, subashab,
abhishek.esse, netdev, devicetree, linux-kernel, linux-soc,
linux-arm-kernel, linux-arm-msm
In-Reply-To: <f0fcee096d779837abc46e7badae9105ee8aaecf.camel@redhat.com>
On 6/24/19 2:54 PM, Dan Williams wrote:
> On Mon, 2019-06-24 at 11:30 -0500, Alex Elder wrote:
>> OK I want to try to organize a little more concisely some of the
>> discussion on this, because there is a very large amount of volume
>> to date and I think we need to try to narrow the focus back down
>> again.
>>
>> I'm going to use a few terms here. Some of these I really don't
>> like, but I want to be unambiguous *and* (at least for now) I want
>> to avoid the very overloaded term "device".
>>
>> I have lots more to say, but let's start with a top-level picture,
>> to make sure we're all on the same page.
>>
>> WWAN Communication
>> Channel (Physical)
>> | ------------------------
>> ------------ v | :+ Control | \
>>> |-----------| :+ Data | |
>>> AP | | WWAN unit :+ Voice | > Functions
>>> |===========| :+ GPS | |
>> ------------ ^ | :+ ... | /
>> | -------------------------
>> Multiplexed WWAN
>> Communication
>> Channel (Physical)
>>
>> - The *AP* is the main CPU complex that's running Linux on one or
>> more CPU cores.
>> - A *WWAN unit* is an entity that shares one or more physical
>> *WWAN communication channels* with the AP.
>
> You could just say "WWAN modem" here.
That sounds great to me.
>> - A *WWAN communication channel* is a bidirectional means of
>> carrying data between the AP and WWAN unit.
>> - A WWAN communication channel carries data using a *WWAN protocol*.
>> - A WWAN unit implements one or more *WWAN functions*, such as
>> 5G data, LTE voice, GPS, and so on.
>
> Go more generic here. Not just 5G data but any WWAN IP-based data
> (GPRS, EDGE, CDMA, UMTS, EVDO, LTE, 5G, etc). And not just LTE voice
> but any voice data; plenty of devices don't support LTE but still have
> "WWAN logical communication channels"
I really meant *any* sort of function, and was only trying
to give a few examples. So yes, my meaning was completely
generic, as you suggest.
>> - A WWAN unit shall implement a *WWAN control function*, used to
>> manage the use of other WWAN functions, as well as the WWAN unit
>> itself.
>> - The AP communicates with a WWAN function using a WWAN protocol.
>> - A WWAN physical channel can be *multiplexed*, in which case it
>> carries the data for one or more *WWAN logical channels*.
>
> It's unclear to me what "physical" means here. USB Interface or
> Endpoint or PCI Function or SMD channel? Or kernel TTY device?
I'm trying to distinguish between (let's say) "hardware" communication
channels (such as what IPA basically provides) and logical ones,
whose data is multiplexed over a "hardware"/"physical" channel.
Maybe "link" would be a better term for what I referred to as a
physical channel, and then have "channels" be multiplexed over a link.
If you have a good suggestion, please offer it.
But I think yes, USB interface, TTY device, etc. are what I mean.
I wanted to capture the fact that there might be more than one of
these (for example a user space QMI link for control, and IPA link
for data?), and that any one of these might also be multiplexed.
Multiplexing is a pretty basic capability of a network link, and I
think the only reason I called it out separately is to be explicit
that it is needs to be supported.
> For example on Qualcomm-based USB dongles a given USB Interface's
> Endpoint represents a QMAP "IP data" channel which itself could be
> multiplexed into separate "IP data" channels. Or that USB Endpoint(s)
> could be exposed as a TTY which itself can be MUX-ed dynamically using
> GSM 07.10.
Yeah. In this case the hardware connection between the AP and the
USB dongle would provide a WWAN link (which I think corresponds to
the QMAP "IP data" channel). And if you wanted to multiplex that
you would use a multiplexing protocol (like QMAP). And that protocol
would carry one or more logical channels, each using its own WWAN
protocol. It sounds like GSM 07.10 would be another WWAN multiplexing
protocol.
> To me "physical" usually means the bus type (PCI, USB, SMD, whatever).
> A Linux hardware driver (IPA, qmi_wwan, option, sierra, etc) binds to
> that physical entity using hardware IDs (USB or PCI VID/PID, devicetree
> properties) and exposes some "WWAN logical communication channels".
(Or perhaps exposes the ability to create "WWAN logical channels.")
"Physical" is probably not a good term. And perhaps focusing
on the transport isn't right--maybe the focus should mainly be on
the WWAN modem entity. But one of the things we're trying to
address here is that there might be several distinct "physical"
paths through which the AP and modem can communicate, so a driver's
ability to provide such a path should be a sort of first class
abstraction.
I'm really trying to get this discussion to converge a little, to
have a few anchor points to build on. I hope we're getting there.
> Those logical channels might be multiplexed and another driver (rmnet)
> could handle exposing the de-muxed logical channels that the muxed
> logical channel carries.
>
>> - A multiplexed WWAN communication channel uses a *WWAN wultiplexing
>> protocol*, which is used to separate independent data streams
>> carrying other WWAN protocols.
>> - A WWAN logical channel carries a bidirectional stream of WWAN
>> protocol data between an entity on the AP and a WWAN function.
>
> It *usually* is bidirectional. For example some GPS logical
> communication channels just start spitting out NMEA when you give the
> control function a command. The NMEA ports themselves don't accept any
> input.
That's fine... I don't think there's anything wrong with a
particular application not using both directions.
>> Does that adequately represent a very high-level picture of what
>> we're trying to manage?
>
> Yes, pretty well. Thanks for trying to specify it all.
I think we're making progress. I have some more thoughts but I
think I'll wait until tomorrow to share them.
Thanks a lot Dan. At some point it might be better to have a
conference call to make better progress, but I'm not suggesting
that yet.
-Alex
>> And if I understand it right, the purpose of the generic framework
>> being discussed is to define a common mechanism for managing (i.e.,
>> discovering, creating, destroying, querying, configuring, enabling,
>> disabling, etc.) WWAN units and the functions they implement, along
>> with the communication and logical channels used to communicate with
>> them.
>
> Yes.
>
> Dan
>
>> Comments?
>>
>> -Alex
>
^ permalink raw reply
* Re: [PATCH] FDDI: defza: Include linux/io-64-nonatomic-lo-hi.h
From: Paul Burton @ 2019-06-24 21:16 UTC (permalink / raw)
To: Paul Burton
Cc: Maciej W. Rozycki, David S. Miller, netdev@vger.kernel.org,
Paul Burton, Serge Semin, linux-mips@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org
In-Reply-To: <20190620221224.27352-1-paul.burton@mips.com>
Hello,
Paul Burton wrote:
> Currently arch/mips/include/asm/io.h provides 64b memory accessor
> functions such as readq & writeq even on MIPS32 platforms where those
> accessors cannot actually perform a 64b memory access. They instead
> BUG(). This is unfortunate for drivers which either #ifdef on the
> presence of these accessors, or can function with non-atomic
> implementations of them found in either linux/io-64-nonatomic-lo-hi.h or
> linux/io-64-nonatomic-hi-lo.h. As such we're preparing to remove the
> definitions of these 64b accessor functions for MIPS32 kernels.
>
> In preparation for this, include linux/io-64-nonatomic-lo-hi.h in
> defza.c in order to provide a non-atomic implementation of the
> readq_relaxed & writeq_relaxed functions that are used by this code. In
> practice this will have no runtime effect, since use of the 64b accessor
> functions is conditional upon sizeof(unsigned long) == 8, ie. upon
> CONFIG_64BIT=y. This means the calls to these non-atomic readq & writeq
> implementations will be optimized out anyway, but we need their
> definitions to keep the compiler happy.
>
> For 64bit kernels using this code this change should also have no effect
> because asm/io.h will continue to provide the definitions of
> readq_relaxed & writeq_relaxed, which linux/io-64-nonatomic-lo-hi.h
> checks for before defining itself.
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Cc: Serge Semin <Sergey.Semin@t-platforms.ru>
> Cc: "Maciej W. Rozycki" <macro@linux-mips.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-mips@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Maciej W. Rozycki <macro@linux-mips.org>
> Acked-by: David S. Miller <davem@davemloft.net>
Applied to mips-next.
Thanks,
Paul
[ This message was auto-generated; if you believe anything is incorrect
then please email paul.burton@mips.com to report it. ]
^ permalink raw reply
* Re: [PATCH bpf-next v8 1/9] bpf: implement getsockopt and setsockopt hooks
From: Andrii Nakryiko @ 2019-06-24 21:19 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
Daniel Borkmann, Martin Lau
In-Reply-To: <20190624162429.16367-2-sdf@google.com>
On Mon, Jun 24, 2019 at 1:11 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Implement new BPF_PROG_TYPE_CGROUP_SOCKOPT program type and
> BPF_CGROUP_{G,S}ETSOCKOPT cgroup hooks.
>
> BPF_CGROUP_SETSOCKOPT get a read-only view of the setsockopt arguments.
> BPF_CGROUP_GETSOCKOPT can modify the supplied buffer.
> Both of them reuse existing PTR_TO_PACKET{,_END} infrastructure.
>
> The buffer memory is pre-allocated (because I don't think there is
> a precedent for working with __user memory from bpf). This might be
> slow to do for each {s,g}etsockopt call, that's why I've added
> __cgroup_bpf_prog_array_is_empty that exits early if there is nothing
> attached to a cgroup. Note, however, that there is a race between
> __cgroup_bpf_prog_array_is_empty and BPF_PROG_RUN_ARRAY where cgroup
> program layout might have changed; this should not be a problem
> because in general there is a race between multiple calls to
> {s,g}etsocktop and user adding/removing bpf progs from a cgroup.
>
> The return code of the BPF program is handled as follows:
> * 0: EPERM
> * 1: success, continue with next BPF program in the cgroup chain
>
> v8:
> * use s32 for optlen (Andrii Nakryiko)
>
> v7:
> * return only 0 or 1 (Alexei Starovoitov)
> * always run all progs (Alexei Starovoitov)
> * use optval=0 as kernel bypass in setsockopt (Alexei Starovoitov)
> (decided to use optval=-1 instead, optval=0 might be a valid input)
> * call getsockopt hook after kernel handlers (Alexei Starovoitov)
>
> v6:
> * rework cgroup chaining; stop as soon as bpf program returns
> 0 or 2; see patch with the documentation for the details
> * drop Andrii's and Martin's Acked-by (not sure they are comfortable
> with the new state of things)
>
> v5:
> * skip copy_to_user() and put_user() when ret == 0 (Martin Lau)
>
> v4:
> * don't export bpf_sk_fullsock helper (Martin Lau)
> * size != sizeof(__u64) for uapi pointers (Martin Lau)
> * offsetof instead of bpf_ctx_range when checking ctx access (Martin Lau)
>
> v3:
> * typos in BPF_PROG_CGROUP_SOCKOPT_RUN_ARRAY comments (Andrii Nakryiko)
> * reverse christmas tree in BPF_PROG_CGROUP_SOCKOPT_RUN_ARRAY (Andrii
> Nakryiko)
> * use __bpf_md_ptr instead of __u32 for optval{,_end} (Martin Lau)
> * use BPF_FIELD_SIZEOF() for consistency (Martin Lau)
> * new CG_SOCKOPT_ACCESS macro to wrap repeated parts
>
> v2:
> * moved bpf_sockopt_kern fields around to remove a hole (Martin Lau)
> * aligned bpf_sockopt_kern->buf to 8 bytes (Martin Lau)
> * bpf_prog_array_is_empty instead of bpf_prog_array_length (Martin Lau)
> * added [0,2] return code check to verifier (Martin Lau)
> * dropped unused buf[64] from the stack (Martin Lau)
> * use PTR_TO_SOCKET for bpf_sockopt->sk (Martin Lau)
> * dropped bpf_target_off from ctx rewrites (Martin Lau)
> * use return code for kernel bypass (Martin Lau & Andrii Nakryiko)
>
> Cc: Martin Lau <kafai@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply
* [PATCH] sis900: remove TxIDLE
From: Sergej Benilov @ 2019-06-24 21:21 UTC (permalink / raw)
To: venza, netdev; +Cc: Sergej Benilov
Before "sis900: fix TX completion" patch, TX completion was done on TxIDLE interrupt.
TX completion also was the only thing done on TxIDLE interrupt.
Since "sis900: fix TX completion", TX completion is done on TxDESC interrupt.
So it is not necessary any more to set and to check for TxIDLE.
Eliminate TxIDLE from sis900.
Correct some typos, too.
Signed-off-by: Sergej Benilov <sergej.benilov@googlemail.com>
---
drivers/net/ethernet/sis/sis900.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c
index 9b036c857..aba6eea72 100644
--- a/drivers/net/ethernet/sis/sis900.c
+++ b/drivers/net/ethernet/sis/sis900.c
@@ -360,7 +360,7 @@ static int sis635_get_mac_addr(struct pci_dev *pci_dev,
* SiS962 or SiS963 model, use EEPROM to store MAC address. And EEPROM
* is shared by
* LAN and 1394. When access EEPROM, send EEREQ signal to hardware first
- * and wait for EEGNT. If EEGNT is ON, EEPROM is permitted to be access
+ * and wait for EEGNT. If EEGNT is ON, EEPROM is permitted to be accessed
* by LAN, otherwise is not. After MAC address is read from EEPROM, send
* EEDONE signal to refuse EEPROM access by LAN.
* The EEPROM map of SiS962 or SiS963 is different to SiS900.
@@ -882,7 +882,7 @@ static void mdio_reset(struct sis900_private *sp)
* mdio_read - read MII PHY register
* @net_dev: the net device to read
* @phy_id: the phy address to read
- * @location: the phy regiester id to read
+ * @location: the phy register id to read
*
* Read MII registers through MDIO and MDC
* using MDIO management frame structure and protocol(defined by ISO/IEC).
@@ -926,7 +926,7 @@ static int mdio_read(struct net_device *net_dev, int phy_id, int location)
* mdio_write - write MII PHY register
* @net_dev: the net device to write
* @phy_id: the phy address to write
- * @location: the phy regiester id to write
+ * @location: the phy register id to write
* @value: the register value to write with
*
* Write MII registers with @value through MDIO and MDC
@@ -1057,7 +1057,7 @@ sis900_open(struct net_device *net_dev)
sis900_set_mode(sis_priv, HW_SPEED_10_MBPS, FDX_CAPABLE_HALF_SELECTED);
/* Enable all known interrupts by setting the interrupt mask. */
- sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxIDLE | TxDESC);
+ sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxDESC);
sw32(cr, RxENA | sr32(cr));
sw32(ier, IE);
@@ -1101,7 +1101,7 @@ sis900_init_rxfilter (struct net_device * net_dev)
sw32(rfdr, w);
if (netif_msg_hw(sis_priv)) {
- printk(KERN_DEBUG "%s: Receive Filter Addrss[%d]=%x\n",
+ printk(KERN_DEBUG "%s: Receive Filter Address[%d]=%x\n",
net_dev->name, i, sr32(rfdr));
}
}
@@ -1148,7 +1148,7 @@ sis900_init_tx_ring(struct net_device *net_dev)
* @net_dev: the net device to initialize for
*
* Initialize the Rx descriptor ring,
- * and pre-allocate recevie buffers (socket buffer)
+ * and pre-allocate receive buffers (socket buffer)
*/
static void
@@ -1578,7 +1578,7 @@ static void sis900_tx_timeout(struct net_device *net_dev)
sw32(txdp, sis_priv->tx_ring_dma);
/* Enable all known interrupts by setting the interrupt mask. */
- sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxIDLE | TxDESC);
+ sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxDESC);
}
/**
@@ -1674,8 +1674,8 @@ static irqreturn_t sis900_interrupt(int irq, void *dev_instance)
do {
status = sr32(isr);
- if ((status & (HIBERR|TxURN|TxERR|TxIDLE|TxDESC|RxORN|RxERR|RxOK)) == 0)
- /* nothing intresting happened */
+ if ((status & (HIBERR|TxURN|TxERR|TxDESC|RxORN|RxERR|RxOK)) == 0)
+ /* nothing interesting happened */
break;
handled = 1;
@@ -1684,7 +1684,7 @@ static irqreturn_t sis900_interrupt(int irq, void *dev_instance)
/* Rx interrupt */
sis900_rx(net_dev);
- if (status & (TxURN | TxERR | TxIDLE | TxDESC))
+ if (status & (TxURN | TxERR | TxDESC))
/* Tx interrupt */
sis900_finish_xmit(net_dev);
@@ -1897,7 +1897,7 @@ static void sis900_finish_xmit (struct net_device *net_dev)
if (tx_status & OWN) {
/* The packet is not transmitted yet (owned by hardware) !
* Note: this is an almost impossible condition
- * in case of TxDESC ('descriptor interrupt') */
+ * on TxDESC interrupt ('descriptor interrupt') */
break;
}
@@ -2473,7 +2473,7 @@ static int sis900_resume(struct pci_dev *pci_dev)
sis900_set_mode(sis_priv, HW_SPEED_10_MBPS, FDX_CAPABLE_HALF_SELECTED);
/* Enable all known interrupts by setting the interrupt mask. */
- sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxIDLE | TxDESC);
+ sw32(imr, RxSOVR | RxORN | RxERR | RxOK | TxURN | TxERR | TxDESC);
sw32(cr, RxENA | sr32(cr));
sw32(ier, IE);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH V34 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Daniel Borkmann @ 2019-06-24 20:59 UTC (permalink / raw)
To: Andy Lutomirski, Matthew Garrett
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Jann Horn, bpf
In-Reply-To: <CALCETrWmZX3R1L88Gz9vLY68gcK8zSXL4cA4GqAzQoyqSR7rRQ@mail.gmail.com>
On 06/24/2019 10:08 PM, Andy Lutomirski wrote:
> On Mon, Jun 24, 2019 at 12:54 PM Matthew Garrett <mjg59@google.com> wrote:
>> On Mon, Jun 24, 2019 at 8:37 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 06/22/2019 02:03 AM, Matthew Garrett wrote:
>>>> From: David Howells <dhowells@redhat.com>
>>>>
>>>> There are some bpf functions can be used to read kernel memory:
>>>
>>> Nit: that
>>
>> Fixed.
>>
>>>> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk. These allow
>>>
>>> Please explain how bpf_probe_write_user reads kernel memory ... ?!
>>
>> Ha.
>>
>>>> private keys in kernel memory (e.g. the hibernation image signing key) to
>>>> be read by an eBPF program and kernel memory to be altered without
>>>
>>> ... and while we're at it, also how they allow "kernel memory to be
>>> altered without restriction". I've been pointing this false statement
>>> out long ago.
>>
>> Yup. How's the following description:
>>
>> bpf: Restrict bpf when kernel lockdown is in confidentiality mode
>>
>> There are some bpf functions that can be used to read kernel memory and
>> exfiltrate it to userland: bpf_probe_read, bpf_probe_write_user and
>> bpf_trace_printk. These could be abused to (eg) allow private
>> keys in kernel
>> memory to be leaked. Disable them if the kernel has been locked
>> down in confidentiality
>> mode.
>
> I'm confused. I understand why we're restricting bpf_probe_read().
> Why are we restricting bpf_probe_write_user() and bpf_trace_printk(),
> though?
Agree, for example, bpf_probe_write_user() can never write into
kernel memory (only user one). Just thinking out loud, wouldn't it
be cleaner and more generic to perform this check at the actual function
which performs the kernel memory without faulting? All three of these
are in mm/maccess.c, and the very few occasions that override the
probe_kernel_read symbol are calling eventually into __probe_kernel_read(),
so this would catch all of them wrt lockdown restrictions. Otherwise
you'd need to keep tracking every bit of new code being merged that
calls into one of these, no? That way you only need to do it once like
below and are guaranteed that the check catches these in future as well.
Thanks,
Daniel
diff --git a/mm/maccess.c b/mm/maccess.c
index 482d4d6..2c8220f 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -29,6 +29,9 @@ long __probe_kernel_read(void *dst, const void *src, size_t size)
long ret;
mm_segment_t old_fs = get_fs();
+ if (security_locked_down(LOCKDOWN_KERNEL_READ))
+ return -EFAULT;
+
set_fs(KERNEL_DS);
pagefault_disable();
ret = __copy_from_user_inatomic(dst,
@@ -57,6 +60,9 @@ long __probe_kernel_write(void *dst, const void *src, size_t size)
long ret;
mm_segment_t old_fs = get_fs();
+ if (security_locked_down(LOCKDOWN_KERNEL_WRITE))
+ return -EFAULT;
+
set_fs(KERNEL_DS);
pagefault_disable();
ret = __copy_to_user_inatomic((__force void __user *)dst, src, size);
@@ -90,6 +96,9 @@ long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count)
const void *src = unsafe_addr;
long ret;
+ if (security_locked_down(LOCKDOWN_KERNEL_READ))
+ return -EFAULT;
+
if (unlikely(count <= 0))
return 0;
^ permalink raw reply related
* Re: [PATCH V34 23/29] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-06-24 21:30 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andy Lutomirski, James Morris, LSM List,
Linux Kernel Mailing List, Linux API, David Howells,
Alexei Starovoitov, Network Development, Chun-Yi Lee, Jann Horn,
bpf
In-Reply-To: <7f36edf7-3120-975e-b643-3c0fa470bafd@iogearbox.net>
On Mon, Jun 24, 2019 at 2:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> Agree, for example, bpf_probe_write_user() can never write into
> kernel memory (only user one). Just thinking out loud, wouldn't it
> be cleaner and more generic to perform this check at the actual function
> which performs the kernel memory without faulting? All three of these
> are in mm/maccess.c, and the very few occasions that override the
> probe_kernel_read symbol are calling eventually into __probe_kernel_read(),
> so this would catch all of them wrt lockdown restrictions. Otherwise
> you'd need to keep tracking every bit of new code being merged that
> calls into one of these, no? That way you only need to do it once like
> below and are guaranteed that the check catches these in future as well.
Not all paths into probe_kernel_read/write are from entry points that
need to be locked down (eg, as far as I can tell ftrace can't leak
anything interesting here).
^ permalink raw reply
* Re: [PATCH iproute2 0/3] do not set IPv6-only options on IPv4 addresses
From: Andrea Claudi @ 2019-06-24 21:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, David Ahern
In-Reply-To: <20190624102041.25224fae@hermes.lan>
On Mon, Jun 24, 2019 at 7:21 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon, 24 Jun 2019 19:05:52 +0200
> Andrea Claudi <aclaudi@redhat.com> wrote:
>
> > 'home', 'nodad' and 'mngtmpaddr' options are IPv6-only, but
> > it is possible to set them on IPv4 addresses, too. This should
> > not be possible.
> >
> > Fix this adding a check on the protocol family before setting
> > the flags, and exiting with invarg() on error.
> >
> > Andrea Claudi (3):
> > ip address: do not set nodad option for IPv4 addresses
> > ip address: do not set home option for IPv4 addresses
> > ip address: do not set mngtmpaddr option for IPv4 addresses
> >
> > ip/ipaddress.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
>
> Maybe this should be a warning, not a failure.
> A little concerned that there will be some user with a scripted setup
> that this breaks.
Hi Stephen,
I think that if a script wrongly uses some of these flags on a IPv4
address, it most probably operates on an unexpected address, since
everyone is aware that these flags are IPv6 only. In other words we
are breaking a scripted setup that is already broken.
In this case it's probably worth exiting with error and give the
author the chance to fix the script, otherwise the error can go
unnoticed.
If you prefer, I can send a v2 with warnings instead of errors, just
let me know.
Regards,
Andrea
^ permalink raw reply
* Re: [PATCH bpf-next v5 0/3] xdp: Allow lookup into devmaps before redirect
From: Toke Høiland-Jørgensen @ 2019-06-24 21:38 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, Jesper Dangaard Brouer, Daniel Borkmann,
Alexei Starovoitov, David Miller, Jonathan Lemon
In-Reply-To: <CAEf4BzZPb2YsrvOfshJAboY9KE3dCa_FZsTkxvQyPquzDChz+w@mail.gmail.com>
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> On Mon, Jun 24, 2019 at 12:38 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Sat, Jun 22, 2019 at 7:19 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> When using the bpf_redirect_map() helper to redirect packets from XDP, the eBPF
>> >> program cannot currently know whether the redirect will succeed, which makes it
>> >> impossible to gracefully handle errors. To properly fix this will probably
>> >> require deeper changes to the way TX resources are allocated, but one thing that
>> >> is fairly straight forward to fix is to allow lookups into devmaps, so programs
>> >> can at least know when a redirect is *guaranteed* to fail because there is no
>> >> entry in the map. Currently, programs work around this by keeping a shadow map
>> >> of another type which indicates whether a map index is valid.
>> >>
>> >> This series contains two changes that are complementary ways to fix this issue:
>> >>
>> >> - Moving the map lookup into the bpf_redirect_map() helper (and caching the
>> >> result), so the helper can return an error if no value is found in the map.
>> >> This includes a refactoring of the devmap and cpumap code to not care about
>> >> the index on enqueue.
>> >>
>> >> - Allowing regular lookups into devmaps from eBPF programs, using the read-only
>> >> flag to make sure they don't change the values.
>> >>
>> >> The performance impact of the series is negligible, in the sense that I cannot
>> >> measure it because the variance between test runs is higher than the difference
>> >> pre/post series.
>> >>
>> >> Changelog:
>> >>
>> >> v5:
>> >> - Rebase on latest bpf-next.
>> >> - Update documentation for bpf_redirect_map() with the new meaning of flags.
>> >>
>> >> v4:
>> >> - Fix a few nits from Andrii
>> >> - Lose the #defines in bpf.h and just compare the flags argument directly to
>> >> XDP_TX in bpf_xdp_redirect_map().
>> >>
>> >> v3:
>> >> - Adopt Jonathan's idea of using the lower two bits of the flag value as the
>> >> return code.
>> >> - Always do the lookup, and cache the result for use in xdp_do_redirect(); to
>> >> achieve this, refactor the devmap and cpumap code to get rid the bitmap for
>> >> selecting which devices to flush.
>> >>
>> >> v2:
>> >> - For patch 1, make it clear that the change works for any map type.
>> >> - For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return
>> >> value read-only.
>> >>
>> >> ---
>> >>
>> >> Toke Høiland-Jørgensen (3):
>> >> devmap/cpumap: Use flush list instead of bitmap
>> >> bpf_xdp_redirect_map: Perform map lookup in eBPF helper
>> >> devmap: Allow map lookups from eBPF
>> >>
>> >>
>> >> include/linux/filter.h | 1
>> >> include/uapi/linux/bpf.h | 7 ++-
>> >> kernel/bpf/cpumap.c | 106 ++++++++++++++++++++-----------------------
>> >> kernel/bpf/devmap.c | 113 ++++++++++++++++++++++------------------------
>> >> kernel/bpf/verifier.c | 7 +--
>> >> net/core/filter.c | 29 +++++-------
>> >> 6 files changed, 123 insertions(+), 140 deletions(-)
>> >>
>> >
>> >
>> > Looks like you forgot to add my Acked-by's for your patches?
>>
>> Ah yes, did not carry those forward for the individual patches, my
>> apologies. Could you perhaps be persuaded to send a new one (I believe a
>> response to the cover letter acking the whole series would suffice)?
>> I'll make sure to add the carrying forward of acks into my workflow in
>> the future :)
>
> I don't think patchworks captures ack's from cover letter, but let's
> give it a go:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Thanks!
-Toke
^ permalink raw reply
* Re: [PATCH net-next v5 2/7] etf: Add skip_sock_check
From: Jakub Kicinski @ 2019-06-24 21:40 UTC (permalink / raw)
To: Vedang Patel
Cc: netdev, jeffrey.t.kirsher, davem, jhs, xiyou.wangcong, jiri,
intel-wired-lan, vinicius.gomes, l, m-karicheri2, sergei.shtylyov,
eric.dumazet, aaron.f.brown
In-Reply-To: <1561138108-12943-3-git-send-email-vedang.patel@intel.com>
On Fri, 21 Jun 2019 10:28:23 -0700, Vedang Patel wrote:
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 8b2f993cbb77..409d1616472d 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -990,6 +990,7 @@ struct tc_etf_qopt {
> __u32 flags;
> #define TC_ETF_DEADLINE_MODE_ON BIT(0)
> #define TC_ETF_OFFLOAD_ON BIT(1)
> +#define TC_ETF_SKIP_SOCK_CHECK BIT(2)
> };
>
> enum {
I think build bot complained about the code not building on 32bit.
When you respin could you include a patch to remove the uses of BIT()
in UAPI? See: https://www.spinics.net/lists/netdev/msg579344.html
^ permalink raw reply
* Re: [PATCH net-next 13/18] ionic: Add initial ethtool support
From: Shannon Nelson @ 2019-06-24 21:44 UTC (permalink / raw)
To: Michal Kubecek, netdev
In-Reply-To: <20190624072604.GA27240@unicorn.suse.cz>
On 6/24/19 12:26 AM, Michal Kubecek wrote:
> On Fri, Jun 21, 2019 at 03:30:20PM -0700, Shannon Nelson wrote:
>> On 6/20/19 7:32 PM, Michal Kubecek wrote:
>>> On Thu, Jun 20, 2019 at 01:24:19PM -0700, Shannon Nelson wrote:
>>> +
>>> + if (ch->combined_count > lif->ionic->ntxqs_per_lif)
>>> + return -EINVAL;
>>> This has been already checked in ethtool_set_channels().
>> That's what I get for copying from an existing driver. I'll check those and
>> clean them up.
> The checks in general code were only added recently so most drivers
> probably still have their own checks.
>
That would be the reason. Thanks, I'll clean those up.
sln
^ permalink raw reply
* Re: [PATCH net-next 01/18] ionic: Add basic framework for IONIC Network device driver
From: Shannon Nelson @ 2019-06-24 21:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190624130327.2b16d149@cakuba.netronome.com>
On 6/24/19 1:03 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:07 -0700, Shannon Nelson wrote:
>> diff --git a/Documentation/networking/device_drivers/pensando/ionic.rst b/Documentation/networking/device_drivers/pensando/ionic.rst
>> new file mode 100644
>> index 000000000000..84bdf682052b
>> --- /dev/null
>> +++ b/Documentation/networking/device_drivers/pensando/ionic.rst
>> @@ -0,0 +1,75 @@
>> +.. SPDX-License-Identifier: GPL-2.0+
>> +
>> +==========================================================
>> +Linux* Driver for the Pensando(R) Ethernet adapter family
>> +==========================================================
>> +
>> +Pensando Linux Ethernet driver.
>> +Copyright(c) 2019 Pensando Systems, Inc
>> +
>> +Contents
>> +========
>> +
>> +- Identifying the Adapter
>> +- Special Features
>> +- Support
>> +
>> +
> nit: all instances of multiple empty lines in the docs look a bit
> unnecessary
Yep, looks like I missed a couple. I'll check those again.
sln
^ permalink raw reply
* Re: [RFC PATCH net-next 1/1] tc-testing: Scapy plugin and JSON verification for tdc
From: Alexander Aring @ 2019-06-24 21:47 UTC (permalink / raw)
To: Lucas Bates
Cc: netdev, davem, jhs, kernel, xiyou.wangcong, jiri, mleitner,
vladbu, dcaratti
In-Reply-To: <1560133232-17880-1-git-send-email-lucasb@mojatatu.com>
Hi Lucas,
On Sun, Jun 09, 2019 at 10:20:32PM -0400, Lucas Bates wrote:
> This is a draft version of two new tdc features.
>
> First, the scapy plugin. This requires the scapy Python module
> installed on the system (plugin was tested against v2.4.2).
> The intent is to install a given rule (as the command under test)
> and then generate packets to create statistics for that rule.
> The stats are checked in the verify phase.
>
> A new "scapy" entry appears in the test cases, which currently
> have three requirements: the source interface for the packets,
> the number of packets to be sent, and a string that is processed
> by scapy's eval() function to construct the packets.
eval() is not a function of scapy, it is a python thing.
You need to be careful with that, people can introduce weird code. Since
everything is here under review I think it is okay... so far we don't
introduce a tdc restful API for that.
>
> Limitations: For now, only one type of packet can be crafted
> per test case. Also, knowledge of scapy's syntax is required.
>
Why not add a list of eval() strings and send them out in order?
> Secondly, we add JSON processing as a method of performing the
> verification stage. Each test case can now have a "matchPattern"
> or "matchJSON" field which governs the method tdc will use to
> process the results. The intent is to make it easier to handle
> the verify stage by not requiring complex regular expressions
>
> matchJSON has two fields, path and value. Path is a list of
> strings and integers which indicate the path through the nested
> JSON data - an asterisk is also acceptable in place of
> a number if the specific index of a list is unknown.
>
Can you provide an example?
> This structure may not be the best method of handling JSON
> verification - suggestions have been made that include using a
> third party module to process the JSON, but that creates an
> external dependency for tdc.
>
> To try the sample tests in this patch:
>
> 1) Ensure nsPlugin and scapyPlugin are linked in the plugins/
> subdirectory
> 2) Run:
> sudo ./tdc.py -f tc-tests/actions/scapy-example.json -n
>
> The second test is designed to fail.
>
> Comments and discussion are encouraged!
Can you please split these patches according to your commit message.
1. JSON processing (new core feature?)
2. scapy plugin (should not touch core code)
3. scapy example
- Alex
^ permalink raw reply
* Re: [PATCH net-next 02/18] ionic: Add hardware init and device commands
From: Shannon Nelson @ 2019-06-24 21:50 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Andrew Lunn, netdev
In-Reply-To: <20190624131304.78c1a4a9@cakuba.netronome.com>
On 6/24/19 1:13 PM, Jakub Kicinski wrote:
> On Fri, 21 Jun 2019 15:22:22 -0700, Shannon Nelson wrote:
>>>> +static int identity_show(struct seq_file *seq, void *v)
>>>> +{
>>>> + struct ionic *ionic = seq->private;
>>>> + struct identity *ident = &ionic->ident;
>>>> + struct ionic_dev *idev = &ionic->idev;
>>>> +
>>>> + seq_printf(seq, "asic_type: 0x%x\n", idev->dev_info.asic_type);
>>>> + seq_printf(seq, "asic_rev: 0x%x\n", idev->dev_info.asic_rev);
>>>> + seq_printf(seq, "serial_num: %s\n", idev->dev_info.serial_num);
>>>> + seq_printf(seq, "fw_version: %s\n", idev->dev_info.fw_version);
>>>> + seq_printf(seq, "fw_status: 0x%x\n",
>>>> + ioread8(&idev->dev_info_regs->fw_status));
>>>> + seq_printf(seq, "fw_heartbeat: 0x%x\n",
>>>> + ioread32(&idev->dev_info_regs->fw_heartbeat));
>>> devlink just gained a much more flexible version of ethtool -i. Please
>>> remove all this and use that.
>> Yes, we intend to add a devlink interface, it just isn't in this first
>> patchset, which is already plenty big.
> Please take this out of your patch set, we can't be expected to merge
> debugfs implementation of what has proper APIs :/
Got it.
sln
^ permalink raw reply
* Re: [PATCH iproute2 0/3] do not set IPv6-only options on IPv4 addresses
From: David Ahern @ 2019-06-24 21:51 UTC (permalink / raw)
To: Andrea Claudi, Stephen Hemminger; +Cc: netdev, David Ahern
In-Reply-To: <CAPpH65wG9OXhEnXd2LrL0wQc9P4G7MKQjQ4bUTHN1CVqAc6bMg@mail.gmail.com>
On 6/24/19 3:38 PM, Andrea Claudi wrote:
> I think that if a script wrongly uses some of these flags on a IPv4
> address, it most probably operates on an unexpected address, since
> everyone is aware that these flags are IPv6 only. In other words we
> are breaking a scripted setup that is already broken.
> In this case it's probably worth exiting with error and give the
> author the chance to fix the script, otherwise the error can go
> unnoticed.
>
> If you prefer, I can send a v2 with warnings instead of errors, just
> let me know.
Recent changes for strict mode have shown people do interesting things
with scripts and like the silent "ignores". :-(
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-24 21:51 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Takshak Chahande, netdev, ast, rdna, kernel-team,
Stanislav Fomichev
In-Reply-To: <6fe292ee-fff0-119c-8524-e25783901167@iogearbox.net>
On Mon, 24 Jun 2019 16:22:25 +0200, Daniel Borkmann wrote:
> On 06/22/2019 12:33 AM, Takshak Chahande wrote:
> > With different bpf attach_flags available to attach bpf programs specially
> > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
> > bpf-programs available to any sub-cgroups really needs to be available for
> > easy debugging.
> >
> > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
> > bpf-programs to a cgroup but also the inherited ones from parent cgroup.
> >
> > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to
> > list all the effective bpf-programs available for execution at a specified
> > cgroup.
> >
> > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
> > # ./test_cgroup_attach
> >
> > With old bpftool (without -e option):
> >
> > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
> > ID AttachType AttachFlags Name
> > 271 egress multi pkt_cntr_1
> > 272 egress multi pkt_cntr_2
> >
> > Attached new program pkt_cntr_4 in cg2 gives following:
> >
> > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > ID AttachType AttachFlags Name
> > 273 egress override pkt_cntr_4
> >
> > And with new "-e" option it shows all effective programs for cg2:
> >
> > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > ID AttachType AttachFlags Name
> > 273 egress override pkt_cntr_4
> > 271 egress override pkt_cntr_1
> > 272 egress override pkt_cntr_2
> >
> > Signed-off-by: Takshak Chahande <ctakshak@fb.com>
> > Acked-by: Andrey Ignatov <rdna@fb.com>
>
> Applied, thanks!
This is a cgroup-specific flag, right? It should be a parameter
to cgroup show, not a global flag. Can we please drop this patch
from the tree?
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-24 21:51 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-JE9DEbmh6hJEN=DEdc+SCz_5Lv74mngPBuv=4nNH=zxQ@mail.gmail.com>
On Mon, Jun 24, 2019 at 02:08:43PM -0400, Willem de Bruijn wrote:
> On Sun, Jun 23, 2019 at 8:46 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > When an application is run that:
> > a) Sets its scheduler to be SCHED_FIFO
> > and
> > b) Opens a memory mapped AF_PACKET socket, and sends frames with the
> > MSG_DONTWAIT flag cleared, its possible for the application to hang
> > forever in the kernel. This occurs because when waiting, the code in
> > tpacket_snd calls schedule, which under normal circumstances allows
> > other tasks to run, including ksoftirqd, which in some cases is
> > responsible for freeing the transmitted skb (which in AF_PACKET calls a
> > destructor that flips the status bit of the transmitted frame back to
> > available, allowing the transmitting task to complete).
> >
> > However, when the calling application is SCHED_FIFO, its priority is
> > such that the schedule call immediately places the task back on the cpu,
> > preventing ksoftirqd from freeing the skb, which in turn prevents the
> > transmitting task from detecting that the transmission is complete.
> >
> > We can fix this by converting the schedule call to a completion
> > mechanism. By using a completion queue, we force the calling task, when
> > it detects there are no more frames to send, to schedule itself off the
> > cpu until such time as the last transmitted skb is freed, allowing
> > forward progress to be made.
> >
> > Tested by myself and the reporter, with good results
> >
> > Appies to the net tree
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > Reported-by: Matteo Croce <mcroce@redhat.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> >
> > Change Notes:
> >
> > V1->V2:
> > Enhance the sleep logic to support being interruptible and
> > allowing for honoring to SK_SNDTIMEO (Willem de Bruijn)
> >
> > V2->V3:
> > Rearrage the point at which we wait for the completion queue, to
> > avoid needing to check for ph/skb being null at the end of the loop.
> > Also move the complete call to the skb destructor to avoid needing to
> > modify __packet_set_status. Also gate calling complete on
> > packet_read_pending returning zero to avoid multiple calls to complete.
> > (Willem de Bruijn)
> >
> > Move timeo computation within loop, to re-fetch the socket
> > timeout since we also use the timeo variable to record the return code
> > from the wait_for_complete call (Neil Horman)
> > ---
> > net/packet/af_packet.c | 59 +++++++++++++++++++++++++++++++++++++-----
> > net/packet/internal.h | 2 ++
> > 2 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index a29d66da7394..5c48bb7a4fa5 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -380,7 +380,6 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> > WARN(1, "TPACKET version not supported.\n");
> > BUG();
> > }
> > -
>
> Unrelated to this feature
>
Agreed.
> > smp_wmb();
><snip>
>
> > ph = packet_current_frame(po, &po->tx_ring,
> > TP_STATUS_SEND_REQUEST);
> > - if (unlikely(ph == NULL)) {
> > - if (need_wait && need_resched())
> > - schedule();
> > - continue;
>
> Why not keep the test whether the process needs to wait exactly here (A)?
>
As I said in the changelog, I think it makes the code more readable, to
understand that you are waiting for an event to complete after you send the
frame.
> Then no need for packet_next_frame.
>
Thats fair. I still think waiting at the bottom of the loop is more clear, but
it does save a function, so I'll agree to this.
> > - }
> > +
> > + if (unlikely(ph == NULL))
> > + break;
> >
> > skb = NULL;
> > tp_len = tpacket_parse_header(po, ph, size_max, &data);
> > +
>
> Again
>
> > if (tp_len < 0)
> > goto tpacket_error;
> >
> > @@ -2720,6 +2732,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >
> > skb->destructor = tpacket_destruct_skb;
> > __packet_set_status(po, ph, TP_STATUS_SENDING);
> > +
> > + /*
> > + * If we need to wait and we've sent the last frame pending
> > + * transmission in the mmaped buffer, flag that we need to wait
> > + * on those frames to get freed via tpacket_destruct_skb. This
> > + * flag indicates that tpacket_destruct_skb should call complete
> > + * when the packet_pending count reaches zero, and that we need
> > + * to call wait_on_complete_interruptible_timeout below, to make
> > + * sure we pick up the result of that completion
> > + */
> > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > + po->wait_on_complete = 1;
> > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
>
> This resets timeout on every loop. should only set above the loop once.
>
I explained exactly why I did that in the change log. Its because I reuse the
timeout variable to get the return value of the wait_for_complete call.
Otherwise I need to add additional data to the stack, which I don't want to do.
Sock_sndtimeo is an inline function and really doesn't add any overhead to this
path, so I see no reason not to reuse the variable.
> Also, please limit the comments in the code (also below). If every
> patch would add this many lines of comment, the file would be
> enormous. OTOH, it's great to be this explanatory in the git commit,
> which is easily reached for any line with git blame.
>
> > + }
> > +
> > packet_inc_pending(&po->tx_ring);
> >
> > status = TP_STATUS_SEND_REQUEST;
> > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > err = net_xmit_errno(err);
> > if (err && __packet_get_status(po, ph) ==
> > TP_STATUS_AVAILABLE) {
> > + /* re-init completion queue to avoid subsequent fallthrough
> > + * on a future thread calling wait_on_complete_interruptible_timeout
> > + */
> > + po->wait_on_complete = 0;
>
> If setting where sleeping, no need for resetting if a failure happens
> between those blocks.
>
> > + init_completion(&po->skb_completion);
>
> no need to reinit between each use?
>
I explained exactly why I did this in the comment above. We have to set
wait_for_complete prior to calling transmit, so as to ensure that we call
wait_for_completion before we exit the loop. However, in this error case, we
exit the loop prior to calling wait_for_complete, so we need to reset the
completion variable and the wait_for_complete flag. Otherwise we will be in a
case where, on the next entrace to this loop we will have a completion variable
with completion->done > 0, meaning the next wait will be a fall through case,
which we don't want.
> > /* skb was destructed already */
> > skb = NULL;
> > goto out_status;
> > @@ -2740,6 +2772,20 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > }
> > packet_increment_head(&po->tx_ring);
> > len_sum += tp_len;
> > +
> > + if (po->wait_on_complete) {
> > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> > + po->wait_on_complete = 0;
>
> I was going to argue for clearing in tpacket_destruct_skb. But then we
> would have to separate clear on timeout instead of signal, too.
>
> po->wait_on_complete = 1;
> timeo = wait_for_completion...
> po->wait_on_complete = 0;
>
Also, we would have a race condition, since the destructor may be called from
softirq context (the first cause of the bug I'm fixing here), and so if the
packet is freed prior to us checking wait_for_complete in tpacket_snd, we will
be in the above situation again, exiting the loop with a completion variable in
an improper state.
> as the previous version had is fine, as long as the compiler does not
> "optimize" away an assignment. The function call will avoid reordering
> by the cpu, at least. Probably requires WRITE_ONCE/READ_ONCE.
>
> > + if (!timeo) {
> > + /* We timed out, break out and notify userspace */
> > + err = -ETIMEDOUT;
> > + goto out_status;
>
> goto out_put, there is no active ph or skb here
>
Yes, good catch.
> > + } else if (timeo == -ERESTARTSYS) {
> > + err = -ERESTARTSYS;
> > + goto out_status;
> > + }
> > + }
> > +
> > } while (likely((ph != NULL) ||
> > /* Note: packet_read_pending() might be slow if we have
> > * to call it as it's per_cpu variable, but in fast-path
> > @@ -3207,6 +3253,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
> > sock_init_data(sock, sk);
> >
> > po = pkt_sk(sk);
> > + init_completion(&po->skb_completion);
> > sk->sk_family = PF_PACKET;
> > po->num = proto;
> > po->xmit = dev_queue_xmit;
>
> This is basically replacing a busy-wait with schedule() with sleeping
> using wait_for_completion_interruptible_timeout. My main question is
> does this really need to move control flow around and add
> packet_next_frame? If not, especially for net, the shortest, simplest
> change is preferable.
>
Its not replacing a busy wait at all, its replacing a non-blocking schedule with
a blocking schedule (via completion queues). As for control flow, Im not sure I
why you are bound to the existing control flow, and given that we already have
packet_previous_frame, I didn't see anything egregious about adding
packet_next_frame as well, but since you've seen a way to eliminate it, I'm ok
with it.
Neil
^ permalink raw reply
* Re: [PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy
From: Andrew Lunn @ 2019-06-24 21:52 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, f.fainelli, linux, davem, matthias.bgg, vivien.didelot,
frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624145251.4849-6-opensource@vdorst.com>
> +static int mt7530_isolate_ephy(struct dsa_switch *ds,
> + struct device_node *ephy_node)
> +{
> + struct phy_device *phydev = of_phy_find_device(ephy_node);
> + int ret;
> +
> + if (!phydev)
> + return 0;
> +
> + ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));
genphy_suspend() does what you want.
> + if (ret)
> + dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
> + ephy_node->full_name);
> + else
> + dev_info(ds->dev, "Phy %s in isolation mode!\n",
> + ephy_node->full_name);
No need to clog up the system with yet more kernel messages.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 00/18] Add ionic driver
From: David Miller @ 2019-06-24 21:53 UTC (permalink / raw)
To: jakub.kicinski; +Cc: snelson, netdev, andrew
In-Reply-To: <20190624131952.0b90206e@cakuba.netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 24 Jun 2019 13:19:52 -0700
> On Thu, 20 Jun 2019 13:24:06 -0700, Shannon Nelson wrote:
>> 28 files changed, 9970 insertions(+)
>
> Dave, could we consider setting a LoC limit for series and patches?
> I know this is a new driver, but there's gotta be a way to split
> this up more, even if it's painful for the submitter :S
>
> All the debugfs stuff shouldn't be necessary in the first version,
> just looking at first 2 patches...
I hear what you're saying. But I have to balance this with the concern
for creating a barrier for entry to submitting new drivers.
However, looking from another perspective you are right that review
burdon is not purely on a number of patches level, but rather the
product of number of patches and lines per patch and thus LoC.
I'd hate to specify a hard nuber and would rather try to apply
judgment onto individual submissions and deal with it on a case
by case basis.
If someone thinks a submission is too large, anyone can just say that
and we'll see what happens.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 01/18] ionic: Add basic framework for IONIC Network device driver
From: Shannon Nelson @ 2019-06-24 21:54 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Andrew Lunn, netdev
In-Reply-To: <20190624130759.3d413c26@cakuba.netronome.com>
On 6/24/19 1:07 PM, Jakub Kicinski wrote:
> On Fri, 21 Jun 2019 15:13:31 -0700, Shannon Nelson wrote:
>>>> +#define DRV_VERSION "0.11.0-k"
>>> DRV_VERSION is pretty useless. What you really want to know is the
>>> kernel git tree and commit. The big distributions might backport this
>>> version of the driver back to the old kernel with a million
>>> patches. At which point 0.11.0-k tells you nothing much.
>> Yes, any version numbering thing from the big distros is put into
>> question, but I find this number useful to me for tracking what has been
>> put into the upstream kernel. This plus the full kernel version gives
>> me a pretty good idea of what I'm looking at.
> Still, we strongly encourage ditching the driver version.
> It encourages upstream first development model among other benefits.
< insert typical whining about internal vendor needs that have
nothing to do with upstream practices :-) >
Sure.
sln
^ permalink raw reply
* Re: [PATCH iproute2] iproute: Pass RTM_F_CLONED on dump to fetch cached routes to be flushed
From: David Ahern @ 2019-06-24 21:55 UTC (permalink / raw)
To: Stefano Brivio, Stephen Hemminger
Cc: David Miller, Jianlin Shi, Wei Wang, Martin KaFai Lau,
Eric Dumazet, Matti Vaittinen, netdev
In-Reply-To: <7ae318a8b632c216df95362524cd4bb5f4f1f537.1560561439.git.sbrivio@redhat.com>
On 6/14/19 7:33 PM, Stefano Brivio wrote:
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 2b3dcc5dbd53..192442b42062 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1602,6 +1602,16 @@ static int save_route_prep(void)
> return 0;
> }
>
> +static int iproute_flush_flags(struct nlmsghdr *nlh, int reqlen)
rename that to iproute_flush_filter to be consistent with
iproute_dump_filter.
Actually, why can't the flush code use iproute_dump_filter?
> +{
> + struct rtmsg *rtm = NLMSG_DATA(nlh);
> +
> + if (filter.cloned)
> + rtm->rtm_flags |= RTM_F_CLONED;
> +
> + return 0;
> +}
> +
> static int iproute_flush(int family, rtnl_filter_t filter_fn)
> {
> time_t start = time(0);
> @@ -1624,7 +1634,7 @@ static int iproute_flush(int family, rtnl_filter_t filter_fn)
> filter.flushe = sizeof(flushb);
>
> for (;;) {
> - if (rtnl_routedump_req(&rth, family, NULL) < 0) {
> + if (rtnl_routedump_req(&rth, family, iproute_flush_flags) < 0) {
> perror("Cannot send dump request");
> return -2;
> }
>
^ permalink raw reply
* Re: [PATCH v3 net-next 0/4] cxgb4: Reference count MPS TCAM entries within a PF
From: David Miller @ 2019-06-24 21:56 UTC (permalink / raw)
To: rajur; +Cc: netdev, nirranjan, dt
In-Reply-To: <20190624173535.12572-1-rajur@chelsio.com>
From: Raju Rangoju <rajur@chelsio.com>
Date: Mon, 24 Jun 2019 23:05:31 +0530
> Firmware reference counts the MPS TCAM entries by PF and VF,
> but it does not do it for usage within a PF or VF. This patch
> adds the support to track MPS TCAM entries within a PF.
>
> v2->v3:
> Fixed the compiler errors due to incorrect patch
> Also, removed the new blank line at EOF
> v1->v2:
> Use refcount_t type instead of atomic_t for mps reference count
Series applied, th anks.
^ permalink raw reply
* Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy
From: Florian Fainelli @ 2019-06-24 21:56 UTC (permalink / raw)
To: René van Dorst, sean.wang, linux, davem, matthias.bgg,
andrew, vivien.didelot
Cc: frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624145251.4849-5-opensource@vdorst.com>
On 6/24/19 7:52 AM, René van Dorst wrote:
> On some platforum the external phy can only interface to the port 5 of the
> switch because the RGMII TX and RX lines are swapped. But it still can be
> useful to use the internal phy of the switch to act as a WAN port which
> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
> the SOC. This increases the LAN and WAN routing.
>
> By adding the optional property mediatek,ephy-handle, the external phy
> is put in isolation mode when internal phy is connected to 2nd GMAC via
> phy-handle property.
Most platforms we have seen so far implement this logic with a mdio-mux,
can you see if that is possible here? stmmac has plenty of examples like
those.
--
Florian
^ permalink raw reply
* Re: [PATCH rdma-next v1 00/12] DEVX asynchronous events
From: Saeed Mahameed @ 2019-06-24 21:57 UTC (permalink / raw)
To: leon@kernel.org
Cc: Jason Gunthorpe, Yishai Hadas, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, dledford@redhat.com
In-Reply-To: <20190619044557.GA11611@mtr-leonro.mtl.com>
On Wed, 2019-06-19 at 07:45 +0300, Leon Romanovsky wrote:
> On Tue, Jun 18, 2019 at 06:51:45PM +0000, Saeed Mahameed wrote:
> > On Tue, 2019-06-18 at 20:15 +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@mellanox.com>
> > >
> > > Changelog:
> > > v0 -> v1:
> >
> > Normally 1st submission is V1 and 2nd is V2.
> > so this should have been v1->v2.
>
> "Normally" depends on the language you are using. In C, everything
> starts from 0, including version of patches :).
>
You are wrong:
quoting: https://kernelnewbies.org/PatchTipsAndTricks
"For example, if you're sending the second revision of a patch, you
should use [PATCH v2]."
now don't tell me that second revision is actually 3rd revision or 1st
is 2nd :)..
> > For mlx5-next patches:
> >
> > Acked-by: Saeed Mahameed <saeedm@mellanox.com>
>
> Thanks
^ permalink raw reply
* [PATCH] bpf: Allow bpf_skb_event_output for a few prog types
From: allanzhang @ 2019-06-24 21:58 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
In-Reply-To: <20190624215824.118783-1-allanzhang@google.com>
Software event output is only enabled by a few prog types right now (TC,
LWT out, XDP, sockops). Many other skb based prog types need
bpf_skb_event_output to produce software event.
Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
Test bpf code is generated from code snippet:
struct TMP {
uint64_t tmp;
} tt;
tt.tmp = 5;
bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
&tt, sizeof(tt));
return 1;
the bpf assembly from llvm is:
0: b7 02 00 00 05 00 00 00 r2 = 5
1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
2: bf a4 00 00 00 00 00 00 r4 = r10
3: 07 04 00 00 f8 ff ff ff r4 += -8
4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
6: b7 03 00 00 00 00 00 00 r3 = 0
7: b7 05 00 00 08 00 00 00 r5 = 8
8: 85 00 00 00 19 00 00 00 call 25
9: b7 00 00 00 01 00 00 00 r0 = 1
10: 95 00 00 00 00 00 00 00 exit
Patch 1 is enabling code.
Patch 2 is fullly covered selftest code.
Signed-off-by: allanzhang <allanzhang@google.com>
---
net/core/filter.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..b75fcf412628 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5958,6 +5958,8 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_socket_cookie_proto;
case BPF_FUNC_get_socket_uid:
return &bpf_get_socket_uid_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -5978,6 +5980,8 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_storage_get_proto;
case BPF_FUNC_sk_storage_delete:
return &bpf_sk_storage_delete_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_SOCK_CGROUP_DATA
case BPF_FUNC_skb_cgroup_id:
return &bpf_skb_cgroup_id_proto;
@@ -6226,6 +6230,8 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_redirect_map_proto;
case BPF_FUNC_sk_redirect_hash:
return &bpf_sk_redirect_hash_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
--
2.22.0.410.gd8fdbe21b5-goog
^ 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