* [PATCH net-next 0/5] net: phy: aquantia: improve and extend driver
From: Heiner Kallweit @ 2019-02-22 22:45 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
This series improves and extends the Aquantia PHY driver.
Andrew Lunn (1):
net: phy: aquantia: use genphy_c45_an_config_aneg
Heiner Kallweit (4):
net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109
net: phy: don't change modes we don't care about in genphy_c45_read_lpa
net: phy: add genphy_c45_read_status
net: phy: aquantia: use genphy_c45_read_status
drivers/net/phy/aquantia.c | 94 ++++++++++++++++++++++++++------------
drivers/net/phy/phy-c45.c | 35 +++++++++++++-
include/linux/phy.h | 1 +
3 files changed, 99 insertions(+), 31 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Alexei Starovoitov @ 2019-02-22 22:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Miller, Masami Hiramatsu, Steven Rostedt, Andy Lutomirski,
Linux List Kernel Mailing, Ingo Molnar, Andrew Morton, stable,
Changbin Du, Jann Horn, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <CAHk-=wh9uWJaih5i6qBPer5rzcs=A+yb_ejU093b1esAhoFveQ@mail.gmail.com>
On Fri, Feb 22, 2019 at 01:59:10PM -0800, Linus Torvalds wrote:
> On Fri, Feb 22, 2019 at 1:38 PM David Miller <davem@davemloft.net> wrote:
> >
> > Don't be surprised if we see more separation like this in the future too.
>
> Yes, with the whole meltdown fiasco, there's actually more pressure to
> add more support for separation of kernel/user address spaces. As Andy
> pointed out, it's been discussed as a future wish-list for x86-64 too.
>
> But yeah, right now the *common* architectures all distinguish kernel
> and user space by pointers (ie x86-64, arm64 and powerpc).
That's all fine. I'm missing rationale for making probe_kernel_read()
fail on user addresses.
What is fundamentally wrong with a function probe_any_address_read() ?
For context, typical bpf kprobe program looks like this:
#define probe_read(P) \
({typeof(P) val = 0; bpf_probe_read(&val, sizeof(val), &P); val;})
SEC("kprobe/__set_task_comm")
int bpf_prog(struct pt_regs *ctx)
{
struct signal_struct *signal;
struct task_struct *tsk;
char oldcomm[16] = {};
char newcomm[16] = {};
u16 oom_score_adj;
u32 pid;
tsk = (void *)PT_REGS_PARM1(ctx);
pid = probe_read(tsk->pid);
bpf_probe_read(oldcomm, sizeof(oldcomm), &tsk->comm);
bpf_probe_read(newcomm, sizeof(newcomm), (void *)PT_REGS_PARM2(ctx));
signal = probe_read(tsk->signal);
oom_score_adj = probe_read(signal->oom_score_adj);
...
}
where PT_REGS_PARMx macros are defined per architecture.
On x86 it's #define PT_REGS_PARM1(x) ((x)->di)
The program writer has to know the meaning of function arguments.
In this example they need to know that __set_task_comm is defined as
void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) in the kernel.
Right now these programs just call bpf_probe_read() on whatever data
they need to access and not differentiating whether it's user or kernel.
One idea we discussed is to split bpf_probe_read() into kernel_read and user_read
helpers, but in the BPF verifier we cannot determine which address space
the program wants to access. The prog writer needs to manually analyze the program
to use correct one. But mistakes are possible and cannot be fatal.
On the kernel side we have to be safe.
Both probe_kernel_read and probe_user_read must not panic if a pointer
from wrong address space was passed.
Hence my preference is to keep probe_kernel_read as "try read any address".
The function can be renamed to indicate so.
^ permalink raw reply
* [PATCH net-next 1/5] net: phy: aquantia: remove false 5G and 10G speed ability for AQCS109
From: Heiner Kallweit @ 2019-02-22 22:48 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <4ae20155-367d-4b11-63f9-28580b7cf1cb@gmail.com>
AQCS109 belongs to a family of PHY's where certain members don't
support 5G or 10G. However for all members of the family the chip
reports 10G and 5G capability. Therefore remove the not supported
modes for AQCS109.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/aquantia.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index 34be44d00..9661ef4b4 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -152,6 +152,15 @@ static int aqr_read_status(struct phy_device *phydev)
return 0;
}
+static int aqcs109_config_init(struct phy_device *phydev)
+{
+ /* AQCS109 belongs to a chip family partially supporting 10G and 5G.
+ * PMA speed ability bits are the same for all members of the family,
+ * AQCS109 however supports speeds up to 2.5G only.
+ */
+ return phy_set_max_speed(phydev, SPEED_2500);
+}
+
static struct phy_driver aqr_driver[] = {
{
PHY_ID_MATCH_MODEL(PHY_ID_AQ1202),
@@ -208,6 +217,7 @@ static struct phy_driver aqr_driver[] = {
.name = "Aquantia AQCS109",
.aneg_done = genphy_c45_aneg_done,
.get_features = genphy_c45_pma_read_abilities,
+ .config_init = aqcs109_config_init,
.config_aneg = aqr_config_aneg,
.config_intr = aqr_config_intr,
.ack_interrupt = aqr_ack_interrupt,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 2/5] net: phy: aquantia: add support for auto-negotiation configuration
From: Heiner Kallweit @ 2019-02-22 22:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <4ae20155-367d-4b11-63f9-28580b7cf1cb@gmail.com>
From: Andrew Lunn <andrew@lunn.ch>
Make use of the generic c45 code, plus code specific to the Aquantia
phy for 1000BaseT negotiation.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/aquantia.c | 40 +++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index 9661ef4b4..a1846daa3 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -20,6 +20,10 @@
#define PHY_ID_AQCS109 0x03a1b5c2
#define PHY_ID_AQR405 0x03a1b4b0
+#define MDIO_AN_VEND_PROV 0xc400
+#define MDIO_AN_VEND_PROV_1000BASET_FULL BIT(15)
+#define MDIO_AN_VEND_PROV_1000BASET_HALF BIT(14)
+
#define MDIO_AN_TX_VEND_STATUS1 0xc800
#define MDIO_AN_TX_VEND_STATUS1_10BASET (0x0 << 1)
#define MDIO_AN_TX_VEND_STATUS1_100BASETX (0x1 << 1)
@@ -64,10 +68,40 @@
static int aqr_config_aneg(struct phy_device *phydev)
{
- linkmode_copy(phydev->supported, phy_10gbit_features);
- linkmode_copy(phydev->advertising, phydev->supported);
+ bool changed = false;
+ u16 reg;
+ int ret;
- return 0;
+ if (phydev->autoneg == AUTONEG_DISABLE)
+ return genphy_c45_pma_setup_forced(phydev);
+
+ ret = genphy_c45_an_config_aneg(phydev);
+ if (ret < 0)
+ return ret;
+ if (ret > 0)
+ changed = true;
+
+ /* Clause 45 has no standardized support for 1000BaseT, therefore
+ * use vendor registers for this mode.
+ */
+ reg = 0;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ phydev->advertising))
+ reg |= MDIO_AN_VEND_PROV_1000BASET_FULL;
+
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ phydev->advertising))
+ reg |= MDIO_AN_VEND_PROV_1000BASET_HALF;
+
+ ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_VEND_PROV,
+ MDIO_AN_VEND_PROV_1000BASET_HALF |
+ MDIO_AN_VEND_PROV_1000BASET_FULL, reg);
+ if (ret < 0)
+ return ret;
+ if (ret > 0)
+ changed = true;
+
+ return genphy_c45_check_and_restart_aneg(phydev, changed);
}
static int aqr_config_intr(struct phy_device *phydev)
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 3/5] net: phy: don't change modes we don't care about in genphy_c45_read_lpa
From: Heiner Kallweit @ 2019-02-22 22:50 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <4ae20155-367d-4b11-63f9-28580b7cf1cb@gmail.com>
Because 1000BaseT isn't covered by Clause 45, the 1000BaseT flags in
phydev->lp_advertising may have been set based on vendor registers
already. genphy_c45_read_lpa() would clear these flags as of today.
Therefore switch to mii_lpa_mod_linkmode_lpa_t.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy-c45.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index ce5fa5346..65ee33e34 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -268,7 +268,7 @@ int genphy_c45_read_lpa(struct phy_device *phydev)
if (val < 0)
return val;
- mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, val);
+ mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, val);
phydev->pause = val & LPA_PAUSE_CAP ? 1 : 0;
phydev->asym_pause = val & LPA_PAUSE_ASYM ? 1 : 0;
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 5/5] net: phy: aquantia: use genphy_c45_read_status
From: Heiner Kallweit @ 2019-02-22 22:52 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <4ae20155-367d-4b11-63f9-28580b7cf1cb@gmail.com>
Use new function genphy_c45_read_status(). 1000BaseT link partner
advertisement needs to be read from vendor registers.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/aquantia.c | 46 +++++++++++++++-----------------------
1 file changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index a1846daa3..0f0eb5682 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -39,6 +39,10 @@
#define MDIO_AN_TX_VEND_INT_MASK2 0xd401
#define MDIO_AN_TX_VEND_INT_MASK2_LINK BIT(0)
+#define MDIO_AN_RX_LP_STAT1 0xe820
+#define MDIO_AN_RX_LP_STAT1_1000BASET_FULL BIT(15)
+#define MDIO_AN_RX_LP_STAT1_1000BASET_HALF BIT(14)
+
/* Vendor specific 1, MDIO_MMD_VEND1 */
#define VEND1_GLOBAL_INT_STD_STATUS 0xfc00
#define VEND1_GLOBAL_INT_VEND_STATUS 0xfc01
@@ -154,36 +158,22 @@ static int aqr_ack_interrupt(struct phy_device *phydev)
static int aqr_read_status(struct phy_device *phydev)
{
- int reg;
-
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
- if (reg & MDIO_STAT1_LSTATUS)
- phydev->link = 1;
- else
- phydev->link = 0;
-
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
- mdelay(10);
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1);
-
- switch (reg & MDIO_AN_TX_VEND_STATUS1_RATE_MASK) {
- case MDIO_AN_TX_VEND_STATUS1_2500BASET:
- phydev->speed = SPEED_2500;
- break;
- case MDIO_AN_TX_VEND_STATUS1_1000BASET:
- phydev->speed = SPEED_1000;
- break;
- case MDIO_AN_TX_VEND_STATUS1_100BASETX:
- phydev->speed = SPEED_100;
- break;
- default:
- phydev->speed = SPEED_10000;
- break;
+ int val;
+
+ if (phydev->autoneg == AUTONEG_ENABLE) {
+ val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_RX_LP_STAT1);
+ if (val < 0)
+ return val;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ phydev->lp_advertising,
+ val & MDIO_AN_RX_LP_STAT1_1000BASET_FULL);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
+ phydev->lp_advertising,
+ val & MDIO_AN_RX_LP_STAT1_1000BASET_HALF);
}
- phydev->duplex = DUPLEX_FULL;
- return 0;
+ return genphy_c45_read_status(phydev);
}
static int aqcs109_config_init(struct phy_device *phydev)
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 4/5] net: phy: add genphy_c45_read_status
From: Heiner Kallweit @ 2019-02-22 22:51 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <4ae20155-367d-4b11-63f9-28580b7cf1cb@gmail.com>
Similar to genphy_read_status() for Clause 22 add a generic read_status
function for Clause 45.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy-c45.c | 33 +++++++++++++++++++++++++++++++++
include/linux/phy.h | 1 +
2 files changed, 34 insertions(+)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 65ee33e34..49e7cd08b 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -458,6 +458,39 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(genphy_c45_pma_read_abilities);
+/**
+ * genphy_c45_read_status - read PHY status
+ * @phydev: target phy_device struct
+ *
+ * Reads status from PHY and sets phy_device members accordingly.
+ */
+int genphy_c45_read_status(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = genphy_c45_read_link(phydev);
+ if (ret)
+ return ret;
+
+ phydev->speed = SPEED_UNKNOWN;
+ phydev->duplex = DUPLEX_UNKNOWN;
+ phydev->pause = 0;
+ phydev->asym_pause = 0;
+
+ if (phydev->autoneg == AUTONEG_ENABLE) {
+ ret = genphy_c45_read_lpa(phydev);
+ if (ret)
+ return ret;
+
+ phy_resolve_aneg_linkmode(phydev);
+ } else {
+ ret = genphy_c45_read_pma(phydev);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_read_status);
+
/* The gen10g_* functions are the old Clause 45 stub */
int gen10g_config_aneg(struct phy_device *phydev)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8e9fc5764..a05ba366d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1107,6 +1107,7 @@ int genphy_c45_an_config_aneg(struct phy_device *phydev);
int genphy_c45_an_disable_aneg(struct phy_device *phydev);
int genphy_c45_read_mdix(struct phy_device *phydev);
int genphy_c45_pma_read_abilities(struct phy_device *phydev);
+int genphy_c45_read_status(struct phy_device *phydev);
/* The gen10g_* functions are the old Clause 45 stub */
int gen10g_config_aneg(struct phy_device *phydev);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next 2/5] net: phy: aquantia: add support for auto-negotiation configuration
From: Florian Fainelli @ 2019-02-22 23:00 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <5eab0739-e38c-cb33-9314-881b51edcbf9@gmail.com>
On 2/22/19 2:49 PM, Heiner Kallweit wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Make use of the generic c45 code, plus code specific to the Aquantia
> phy for 1000BaseT negotiation.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/aquantia.c | 40 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
> index 9661ef4b4..a1846daa3 100644
> --- a/drivers/net/phy/aquantia.c
> +++ b/drivers/net/phy/aquantia.c
> @@ -20,6 +20,10 @@
> #define PHY_ID_AQCS109 0x03a1b5c2
> #define PHY_ID_AQR405 0x03a1b4b0
>
> +#define MDIO_AN_VEND_PROV 0xc400
> +#define MDIO_AN_VEND_PROV_1000BASET_FULL BIT(15)
> +#define MDIO_AN_VEND_PROV_1000BASET_HALF BIT(14)
> +
> #define MDIO_AN_TX_VEND_STATUS1 0xc800
> #define MDIO_AN_TX_VEND_STATUS1_10BASET (0x0 << 1)
> #define MDIO_AN_TX_VEND_STATUS1_100BASETX (0x1 << 1)
> @@ -64,10 +68,40 @@
>
> static int aqr_config_aneg(struct phy_device *phydev)
> {
> - linkmode_copy(phydev->supported, phy_10gbit_features);
> - linkmode_copy(phydev->advertising, phydev->supported);
> + bool changed = false;
You could entirely eliminate changed since this is just a shorthand for
ret > 0 after your recent changes to return that information. Other than
that:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Jann Horn @ 2019-02-22 23:02 UTC (permalink / raw)
To: Nadav Amit
Cc: Andy Lutomirski, Alexei Starovoitov, Steven Rostedt,
Linus Torvalds, Masami Hiramatsu, Linux List Kernel Mailing,
Ingo Molnar, Andrew Morton, Changbin Du, Kees Cook,
Andy Lutomirski, Daniel Borkmann, Network Development,
bpf@vger.kernel.org, Rick Edgecombe, Dave Hansen,
Peter Zijlstra (Intel), Igor Stoppa
In-Reply-To: <A7232EDD-C53C-4B7D-AD02-4A480F40538B@vmware.com>
On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit <namit@vmware.com> wrote:
> > On Feb 22, 2019, at 2:21 PM, Nadav Amit <namit@vmware.com> wrote:
> >
> >> On Feb 22, 2019, at 2:17 PM, Jann Horn <jannh@google.com> wrote:
> >>
> >> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit <namit@vmware.com> wrote:
> >>>> On Feb 22, 2019, at 1:43 PM, Jann Horn <jannh@google.com> wrote:
> >>>>
> >>>> (adding some people from the text_poke series to the thread, removing stable@)
> >>>>
> >>>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >>>>>> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >>>>>>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote:
> >>>>>>> On Fri, 22 Feb 2019 11:27:05 -0800
> >>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >>>>>>>
> >>>>>>>>> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
> >>>>>>>>>
> >>>>>>>>> Then we should still probably fix up "__probe_kernel_read()" to not
> >>>>>>>>> allow user accesses. The easiest way to do that is actually likely to
> >>>>>>>>> use the "unsafe_get_user()" functions *without* doing a
> >>>>>>>>> uaccess_begin(), which will mean that modern CPU's will simply fault
> >>>>>>>>> on a kernel access to user space.
> >>>>>>>>
> >>>>>>>> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
> >>>>>>>> and users pass both user and kernel addresses into it and expect
> >>>>>>>> that the helper will actually try to read from that address.
> >>>>>>>>
> >>>>>>>> If __probe_kernel_read will suddenly start failing on all user addresses
> >>>>>>>> it will break the expectations.
> >>>>>>>> How do we solve it in bpf_probe_read?
> >>>>>>>> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
> >>>>>>>> in the loop?
> >>>>>>>> That's doable, but people already complain that bpf_probe_read() is slow
> >>>>>>>> and shows up in their perf report.
> >>>>>>>
> >>>>>>> We're changing kprobes to add a specific flag to say that we want to
> >>>>>>> differentiate between kernel or user reads. Can this be done with
> >>>>>>> bpf_probe_read()? If it's showing up in perf report, I doubt a single
> >>>>>>
> >>>>>> so you're saying you will break existing kprobe scripts?
> >>>>>> I don't think it's a good idea.
> >>>>>> It's not acceptable to break bpf_probe_read uapi.
> >>>>>
> >>>>> If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x.
> >>>>>
> >>>>> What to do about existing scripts is a different question.
> >>>>
> >>>> This lack of logical separation between user and kernel addresses
> >>>> might interact interestingly with the text_poke series, specifically
> >>>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for
> >>>> patching" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793&sdata=HAbnDcrBne64JyPuVUMKmM7nQk67F%2BFvjuXEn8TmHeo%3D&reserved=0)
> >>>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text
> >>>> poking" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793&sdata=vNRIMKtFDy%2F3z5FlTwDiJY6VGEV%2FMHgQPTdFSFtCo4s%3D&reserved=0),
> >>>> right? If someone manages to get a tracing BPF program to trigger in a
> >>>> task that has switched to the patching mm, could they use
> >>>> bpf_probe_write_user() - which uses probe_kernel_write() after
> >>>> checking that KERNEL_DS isn't active and that access_ok() passes - to
> >>>> overwrite kernel text that is mapped writable in the patching mm?
> >>>
> >>> Yes, this is a good point. I guess text_poke() should be defined with
> >>> “__kprobes” and open-code memcpy.
> >>>
> >>> Does it sound reasonable?
> >>
> >> Doesn't __text_poke() as implemented in the proposed patch use a
> >> couple other kernel functions, too? Like switch_mm_irqs_off() and
> >> pte_clear() (which can be a call into a separate function on paravirt
> >> kernels)?
> >
> > I will move the pte_clear() to be done after the poking mm was unloaded.
> > Give me a few minutes to send a sketch of what I think should be done.
>
> Err.. You are right, I don’t see an easy way of preventing a kprobe from
> being set on switch_mm_irqs_off(), and open-coding this monster is too ugly.
>
> The reasonable solution seems to me as taking all the relevant pieces of
> code (and data) that might be used during text-poking and encapsulating them, so they
> will be set in a memory area which cannot be kprobe'd. This can also be
> useful to write-protect data structures of code that calls text_poke(),
> e.g., static-keys. It can also protect data on that stack that is used
> during text_poke() from being overwritten from another core.
>
> This solution is somewhat similar to Igor Stoppa’s idea of using “enclaves”
> when doing write-rarely operations.
>
> Right now, I think that text_poke() will keep being susceptible to such
> an attack, unless you have a better suggestion.
A relatively simple approach might be to teach BPF not to run kprobe
programs and such in contexts where current->mm isn't the active mm?
Maybe using nmi_uaccess_okay(), or something like that? It looks like
perf_callchain_user() also already uses that. Except that a lot of
this code is x86-specific...
^ permalink raw reply
* [PATCH bpf] bpf, doc: add bpf list as secondary entry to maintainers file
From: Daniel Borkmann @ 2019-02-22 23:03 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: netdev, Daniel Borkmann
We recently created a bpf@vger.kernel.org list (https://lore.kernel.org/bpf/)
for BPF related discussions, originally in context of BPF track at LSF/MM
for topic discussions. It's *optional* but *desirable* to keep it in Cc for
BPF related kernel/loader/llvm/tooling threads, meaning also infrastructure
like llvm that sits on top of kernel but is crucial to BPF. In any case,
netdev with it's bpf delegate is *as-is* today primary list for patches, so
nothing changes in the workflow. Main purpose is to have some more awareness
for the bpf@vger.kernel.org list that folks can Cc for BPF specific topics.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
MAINTAINERS | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 41ce5f4..d78f371 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2852,7 +2852,7 @@ R: Martin KaFai Lau <kafai@fb.com>
R: Song Liu <songliubraving@fb.com>
R: Yonghong Song <yhs@fb.com>
L: netdev@vger.kernel.org
-L: linux-kernel@vger.kernel.org
+L: bpf@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
T: git git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
Q: https://patchwork.ozlabs.org/project/netdev/list/?delegate=77147
@@ -2882,6 +2882,7 @@ N: bpf
BPF JIT for ARM
M: Shubham Bansal <illusionist.neo@gmail.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/arm/net/
@@ -2890,18 +2891,21 @@ M: Daniel Borkmann <daniel@iogearbox.net>
M: Alexei Starovoitov <ast@kernel.org>
M: Zi Shen Lim <zlim.lnx@gmail.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Supported
F: arch/arm64/net/
BPF JIT for MIPS (32-BIT AND 64-BIT)
M: Paul Burton <paul.burton@mips.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/mips/net/
BPF JIT for NFP NICs
M: Jakub Kicinski <jakub.kicinski@netronome.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Supported
F: drivers/net/ethernet/netronome/nfp/bpf/
@@ -2909,6 +2913,7 @@ BPF JIT for POWERPC (32-BIT AND 64-BIT)
M: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
M: Sandipan Das <sandipan@linux.ibm.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/powerpc/net/
@@ -2916,6 +2921,7 @@ BPF JIT for S390
M: Martin Schwidefsky <schwidefsky@de.ibm.com>
M: Heiko Carstens <heiko.carstens@de.ibm.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/s390/net/
X: arch/s390/net/pnet.c
@@ -2923,12 +2929,14 @@ X: arch/s390/net/pnet.c
BPF JIT for SPARC (32-BIT AND 64-BIT)
M: David S. Miller <davem@davemloft.net>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/sparc/net/
BPF JIT for X86 32-BIT
M: Wang YanQing <udknight@gmail.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: arch/x86/net/bpf_jit_comp32.c
@@ -2936,6 +2944,7 @@ BPF JIT for X86 64-BIT
M: Alexei Starovoitov <ast@kernel.org>
M: Daniel Borkmann <daniel@iogearbox.net>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Supported
F: arch/x86/net/
X: arch/x86/net/bpf_jit_comp32.c
@@ -8487,6 +8496,7 @@ L7 BPF FRAMEWORK
M: John Fastabend <john.fastabend@gmail.com>
M: Daniel Borkmann <daniel@iogearbox.net>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: include/linux/skmsg.h
F: net/core/skmsg.c
@@ -16714,6 +16724,7 @@ M: Jesper Dangaard Brouer <hawk@kernel.org>
M: John Fastabend <john.fastabend@gmail.com>
L: netdev@vger.kernel.org
L: xdp-newbies@vger.kernel.org
+L: bpf@vger.kernel.org
S: Supported
F: net/core/xdp.c
F: include/net/xdp.h
@@ -16727,6 +16738,7 @@ XDP SOCKETS (AF_XDP)
M: Björn Töpel <bjorn.topel@intel.com>
M: Magnus Karlsson <magnus.karlsson@intel.com>
L: netdev@vger.kernel.org
+L: bpf@vger.kernel.org
S: Maintained
F: kernel/bpf/xskmap.c
F: net/xdp/
--
2.9.5
^ permalink raw reply related
* Re: [PATCH][net-next] ipv6: sanitize RCU usage on fib6_next
From: Eric Dumazet @ 2019-02-22 23:08 UTC (permalink / raw)
To: Li RongQing, netdev
In-Reply-To: <1550828682-10608-1-git-send-email-lirongqing@baidu.com>
On 02/22/2019 01:44 AM, Li RongQing wrote:
> using rcu_assign_pointer when setting, which has a memory
> barrier to ensure the initialization is seen first.
>
> using rcu_dereference when dereference this pointer
>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> net/ipv6/ip6_fib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 6613d8dbb0e5..b73d40d68178 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1143,7 +1143,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>
> atomic_inc(&rt->fib6_ref);
> rcu_assign_pointer(rt->fib6_node, fn);
> - rt->fib6_next = iter->fib6_next;
> + rcu_assign_pointer(rt->fib6_next, iter->fib6_next);
We do not need a barrier here, the object is still private.
> rcu_assign_pointer(*ins, rt);
> if (!info->skip_notify)
> inet6_rt_notify(RTM_NEWROUTE, rt, info, NLM_F_REPLACE);
> @@ -1761,7 +1761,9 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
> RT6_TRACE("fib6_del_route\n");
>
> /* Unlink it */
> - *rtp = rt->fib6_next;
> + *rtp = rcu_dereference_protected(rt->fib6_next,
> + lockdep_is_held(&rt->fib6_table->tb6_lock));
This change will likely add a sparse bug.
> +
> rt->fib6_node = NULL;
> net->ipv6.rt6_stats->fib_rt_entries--;
> net->ipv6.rt6_stats->fib_discarded_routes++;
>
Really I do not believe these changes are needed.
barriers should be added blindly.
^ permalink raw reply
* Re: [PATCH bpf 0/2] nfp: bpf: fix two ALU32 code-gen bugs
From: Daniel Borkmann @ 2019-02-22 23:09 UTC (permalink / raw)
To: Jiong Wang, alexei.starovoitov; +Cc: netdev, oss-drivers
In-Reply-To: <1550874964-16769-1-git-send-email-jiong.wang@netronome.com>
On 02/22/2019 11:36 PM, Jiong Wang wrote:
> code-gen for BPF_ALU | BPF_XOR | BPF_K is wrong when imm is -1, also high
> 32-bit of 64-bit register should always be cleared.
>
> This set fixed both bugs.
>
> Jiong Wang (2):
> nfp: bpf: fix code-gen bug on BPF_ALU | BPF_XOR | BPF_K
> nfp: bpf: fix ALU32 high bits clearance bug
>
> drivers/net/ethernet/netronome/nfp/bpf/jit.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Jann Horn @ 2019-02-22 23:11 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, David Miller, Masami Hiramatsu, Steven Rostedt,
Andy Lutomirski, Linux List Kernel Mailing, Ingo Molnar,
Andrew Morton, stable, Changbin Du, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <20190222225103.o5rr5zr4fq77jdg4@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 11:51 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Feb 22, 2019 at 01:59:10PM -0800, Linus Torvalds wrote:
> > On Fri, Feb 22, 2019 at 1:38 PM David Miller <davem@davemloft.net> wrote:
> > >
> > > Don't be surprised if we see more separation like this in the future too.
> >
> > Yes, with the whole meltdown fiasco, there's actually more pressure to
> > add more support for separation of kernel/user address spaces. As Andy
> > pointed out, it's been discussed as a future wish-list for x86-64 too.
> >
> > But yeah, right now the *common* architectures all distinguish kernel
> > and user space by pointers (ie x86-64, arm64 and powerpc).
>
> That's all fine. I'm missing rationale for making probe_kernel_read()
> fail on user addresses.
> What is fundamentally wrong with a function probe_any_address_read() ?
I think what Linus is saying is: There are some scenarios (like a
system with the old 4G/4G X86 patch) where *the same* address can
refer to two different pieces of memory, depending on whether you
interpret it as a kernel pointer or a user pointer. So for example, if
your BPF program tries to read tsk->comm, it works, but if the BPF
program then tries to read from PT_REGS_PARM2(ctx), it's going to
actually interpret the userspace address as a kernel address and read
completely different memory.
On top of that, from the security angle, this means that if a user
passes a kernel pointer into a syscall, they can trick a tracing BPF
program into looking at random kernel memory instead of the user's
memory. That may or may not be problematic, depending on what you do
afterwards with the data you've read. (For example, if this is a
service that collects performance data and then saves it to some
world-readable location on disk because the data it is collecting
(including comm strings) isn't supposed to be sensitive, you might
have a problem.)
> For context, typical bpf kprobe program looks like this:
> #define probe_read(P) \
> ({typeof(P) val = 0; bpf_probe_read(&val, sizeof(val), &P); val;})
> SEC("kprobe/__set_task_comm")
> int bpf_prog(struct pt_regs *ctx)
> {
> struct signal_struct *signal;
> struct task_struct *tsk;
> char oldcomm[16] = {};
> char newcomm[16] = {};
> u16 oom_score_adj;
> u32 pid;
>
> tsk = (void *)PT_REGS_PARM1(ctx);
> pid = probe_read(tsk->pid);
> bpf_probe_read(oldcomm, sizeof(oldcomm), &tsk->comm);
> bpf_probe_read(newcomm, sizeof(newcomm), (void *)PT_REGS_PARM2(ctx));
> signal = probe_read(tsk->signal);
> oom_score_adj = probe_read(signal->oom_score_adj);
> ...
> }
>
> where PT_REGS_PARMx macros are defined per architecture.
> On x86 it's #define PT_REGS_PARM1(x) ((x)->di)
>
> The program writer has to know the meaning of function arguments.
> In this example they need to know that __set_task_comm is defined as
> void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) in the kernel.
>
> Right now these programs just call bpf_probe_read() on whatever data
> they need to access and not differentiating whether it's user or kernel.
>
> One idea we discussed is to split bpf_probe_read() into kernel_read and user_read
> helpers, but in the BPF verifier we cannot determine which address space
> the program wants to access. The prog writer needs to manually analyze the program
> to use correct one. But mistakes are possible and cannot be fatal.
> On the kernel side we have to be safe.
> Both probe_kernel_read and probe_user_read must not panic if a pointer
> from wrong address space was passed.
>
> Hence my preference is to keep probe_kernel_read as "try read any address".
> The function can be renamed to indicate so.
>
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: David Miller @ 2019-02-22 23:16 UTC (permalink / raw)
To: jannh
Cc: alexei.starovoitov, torvalds, mhiramat, rostedt, luto,
linux-kernel, mingo, akpm, stable, changbin.du, keescook, luto,
daniel, netdev, bpf
In-Reply-To: <CAG48ez0nMEMP5-eSFWmLKs30zApgZ57zqU1Od9cROBYQvoJxsg@mail.gmail.com>
From: Jann Horn <jannh@google.com>
Date: Sat, 23 Feb 2019 00:11:58 +0100
> I think what Linus is saying is: There are some scenarios (like a
> system with the old 4G/4G X86 patch) where *the same* address can
> refer to two different pieces of memory, depending on whether you
> interpret it as a kernel pointer or a user pointer.
Exactly.
On sparc64 the kernel is mapped exactly at the same virtual addresses
as userspace processes usually are mapped, even 32-bit ones. The
difference is the MMU context only.
^ permalink raw reply
* Re: [PATCH net 0/2] bnxt_en: firmware message delay fixes.
From: David Miller @ 2019-02-22 23:21 UTC (permalink / raw)
To: michael.chan; +Cc: netdev
In-Reply-To: <1550707652-14747-1-git-send-email-michael.chan@broadcom.com>
From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 20 Feb 2019 19:07:30 -0500
> We were seeing some intermittent firmware message timeouts in our lab and
> these 2 small patches fix them. Please apply to stable as well. Thanks.
Applied and queued up, thanks!
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Linus Torvalds @ 2019-02-22 23:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, Masami Hiramatsu, Steven Rostedt, Andy Lutomirski,
Linux List Kernel Mailing, Ingo Molnar, Andrew Morton, stable,
Changbin Du, Jann Horn, Kees Cook, Andrew Lutomirski,
Daniel Borkmann, Netdev, bpf
In-Reply-To: <20190222225103.o5rr5zr4fq77jdg4@ast-mbp.dhcp.thefacebook.com>
On Fri, Feb 22, 2019 at 2:51 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> That's all fine. I'm missing rationale for making probe_kernel_read()
> fail on user addresses.
Because it already WON'T WORK in general!
> What is fundamentally wrong with a function probe_any_address_read() ?
What part of "the same pointer value can be a user address and a
kernel address" is not getting through?
The user address space and the kernel address space have separate page
tables on some architectures. We used to avoid it on x86, because
switching address spaces was expensive, but even on x86 some vendors
did it on 32-bit simply to get 4GB of user (and kernel) address space.
And now we end up doing it anyway just because of meltdown.
So a kernel pointer value of 0x12345678 could be a value kernel
pointer pointing to some random kmalloc'ed kernel memory, and a user
pointer value of 0x12345678 could be a valid _user_ pointer pointing
to some user mapping.
See?
If you access a user pointer, you need to use a user accessor function
(eg "get_user()"), while if you access a kernel pointer you need to
just dereference it directly (unless you can't trust it, in which case
you need to use a _different_ accessor function).
The fact that user and kernel pointers happen to be distinct on x86-64
(right now) is just a random implementation detail.
Really.
I didn't realize how many people seem to have been confused about
this. But it's always been true. It's just that the common
architectures have had that "one single address space for both kernel
and user pointers" in practice.
In fact, the *very* first kernel version had separate address spaces
for kernel and user mode even on x86 (using segments, not paging). So
it has literally been true since day one in Linux that a kernel
address can be indistinguishable from a user address from a pure value
standpoint.
Linus
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Nadav Amit @ 2019-02-22 23:22 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, Alexei Starovoitov, Steven Rostedt,
Linus Torvalds, Masami Hiramatsu, Linux List Kernel Mailing,
Ingo Molnar, Andrew Morton, Changbin Du, Kees Cook,
Andy Lutomirski, Daniel Borkmann, Network Development,
bpf@vger.kernel.org, Rick Edgecombe, Dave Hansen,
Peter Zijlstra (Intel), Igor Stoppa
In-Reply-To: <CAG48ez1Dm04kg2x3O=OOmBFFBSDr+RmnhBYChcwYv5zLbPGudQ@mail.gmail.com>
> On Feb 22, 2019, at 3:02 PM, Jann Horn <jannh@google.com> wrote:
>
> On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit <namit@vmware.com> wrote:
>>> On Feb 22, 2019, at 2:21 PM, Nadav Amit <namit@vmware.com> wrote:
>>>
>>>> On Feb 22, 2019, at 2:17 PM, Jann Horn <jannh@google.com> wrote:
>>>>
>>>> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit <namit@vmware.com> wrote:
>>>>>> On Feb 22, 2019, at 1:43 PM, Jann Horn <jannh@google.com> wrote:
>>>>>>
>>>>>> (adding some people from the text_poke series to the thread, removing stable@)
>>>>>>
>>>>>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>>>>>>> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote:
>>>>>>>>> On Fri, 22 Feb 2019 11:27:05 -0800
>>>>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>>> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
>>>>>>>>>>>
>>>>>>>>>>> Then we should still probably fix up "__probe_kernel_read()" to not
>>>>>>>>>>> allow user accesses. The easiest way to do that is actually likely to
>>>>>>>>>>> use the "unsafe_get_user()" functions *without* doing a
>>>>>>>>>>> uaccess_begin(), which will mean that modern CPU's will simply fault
>>>>>>>>>>> on a kernel access to user space.
>>>>>>>>>>
>>>>>>>>>> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
>>>>>>>>>> and users pass both user and kernel addresses into it and expect
>>>>>>>>>> that the helper will actually try to read from that address.
>>>>>>>>>>
>>>>>>>>>> If __probe_kernel_read will suddenly start failing on all user addresses
>>>>>>>>>> it will break the expectations.
>>>>>>>>>> How do we solve it in bpf_probe_read?
>>>>>>>>>> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
>>>>>>>>>> in the loop?
>>>>>>>>>> That's doable, but people already complain that bpf_probe_read() is slow
>>>>>>>>>> and shows up in their perf report.
>>>>>>>>>
>>>>>>>>> We're changing kprobes to add a specific flag to say that we want to
>>>>>>>>> differentiate between kernel or user reads. Can this be done with
>>>>>>>>> bpf_probe_read()? If it's showing up in perf report, I doubt a single
>>>>>>>>
>>>>>>>> so you're saying you will break existing kprobe scripts?
>>>>>>>> I don't think it's a good idea.
>>>>>>>> It's not acceptable to break bpf_probe_read uapi.
>>>>>>>
>>>>>>> If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x.
>>>>>>>
>>>>>>> What to do about existing scripts is a different question.
>>>>>>
>>>>>> This lack of logical separation between user and kernel addresses
>>>>>> might interact interestingly with the text_poke series, specifically
>>>>>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for
>>>>>> patching" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cd03df2db76624da8eb2008d69919e41a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864733821233906&sdata=ky5iTrsCceoPwVW5N9FB4sDspwGEQ8MTlRE4b1Bqn54%3D&reserved=0)
>>>>>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text
>>>>>> poking" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cd03df2db76624da8eb2008d69919e41a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864733821233906&sdata=EJs8doLrfFfMTKiVHmWjmpnpWolmuuZ5pxOmEMcI0ew%3D&reserved=0),
>>>>>> right? If someone manages to get a tracing BPF program to trigger in a
>>>>>> task that has switched to the patching mm, could they use
>>>>>> bpf_probe_write_user() - which uses probe_kernel_write() after
>>>>>> checking that KERNEL_DS isn't active and that access_ok() passes - to
>>>>>> overwrite kernel text that is mapped writable in the patching mm?
>>>>>
>>>>> Yes, this is a good point. I guess text_poke() should be defined with
>>>>> “__kprobes” and open-code memcpy.
>>>>>
>>>>> Does it sound reasonable?
>>>>
>>>> Doesn't __text_poke() as implemented in the proposed patch use a
>>>> couple other kernel functions, too? Like switch_mm_irqs_off() and
>>>> pte_clear() (which can be a call into a separate function on paravirt
>>>> kernels)?
>>>
>>> I will move the pte_clear() to be done after the poking mm was unloaded.
>>> Give me a few minutes to send a sketch of what I think should be done.
>>
>> Err.. You are right, I don’t see an easy way of preventing a kprobe from
>> being set on switch_mm_irqs_off(), and open-coding this monster is too ugly.
>>
>> The reasonable solution seems to me as taking all the relevant pieces of
>> code (and data) that might be used during text-poking and encapsulating them, so they
>> will be set in a memory area which cannot be kprobe'd. This can also be
>> useful to write-protect data structures of code that calls text_poke(),
>> e.g., static-keys. It can also protect data on that stack that is used
>> during text_poke() from being overwritten from another core.
>>
>> This solution is somewhat similar to Igor Stoppa’s idea of using “enclaves”
>> when doing write-rarely operations.
>>
>> Right now, I think that text_poke() will keep being susceptible to such
>> an attack, unless you have a better suggestion.
>
> A relatively simple approach might be to teach BPF not to run kprobe
> programs and such in contexts where current->mm isn't the active mm?
> Maybe using nmi_uaccess_okay(), or something like that? It looks like
> perf_callchain_user() also already uses that. Except that a lot of
> this code is x86-specific…
I keep having in mind how to reduce the TCB that is used while text_poke()
is running, but for the specific issue here, I think your approach would
be fine, and trace_call_bpf() can be modified to do what you ask for.
But, I am not sure that relying on current->mm gets us any more security,
relatively to having another unrelated explicit kprobe-disable indication,
which is cleaner from design point-of-view.
I can see how we get “some more security” if our decision whether kprobes
should be enabled was purely based on some hardware register (e.g., CR3) and
we could unequivocally realize whether kprobes eBPF should be on/off without
memory accesses (e.g., PCID bit set). Yet, I am not sure it worth it.
What do you say?
^ permalink raw reply
* Re: [PATCH] Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255
From: David Miller @ 2019-02-22 23:22 UTC (permalink / raw)
To: kalash; +Cc: netdev, linux-kernel
In-Reply-To: <20190221002304.GA24732@us178.sjc.aristanetworks.com>
From: Kalash Nainwal <kalash@arista.com>
Date: Wed, 20 Feb 2019 16:23:04 -0800
> Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255 to
> keep legacy software happy. This is similar to what was done for
> ipv4 in commit 709772e6e065 ("net: Fix routing tables with
> id > 255 for legacy software").
>
> Signed-off-by: Kalash Nainwal <kalash@arista.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [tipc-discussion][net v2 1/1] tipc: fix race condition causing hung sendto
From: David Miller @ 2019-02-22 23:24 UTC (permalink / raw)
To: tung.q.nguyen; +Cc: netdev, tipc-discussion
In-Reply-To: <20190221033121.4220-1-tung.q.nguyen@dektech.com.au>
From: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Date: Thu, 21 Feb 2019 10:31:21 +0700
> When sending multicast messages via blocking socket,
This patch does not apply cleanly to net, please respin:
[davem@localhost net]$ git am --signoff tipc-discussion-net-v2-1-1-tipc-fix-race-condition-causing-hung-sendto.patch
Applying: tipc: fix race condition causing hung sendto
error: patch failed: net/tipc/socket.c:379
error: net/tipc/socket.c: patch does not apply
Patch failed at 0001 tipc: fix race condition causing hung sendto
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply
* Re: [Patch net-next] net_sched: initialize net pointer inside tcf_exts_init()
From: David Miller @ 2019-02-22 23:27 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs, jiri
In-Reply-To: <20190221053742.30907-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 20 Feb 2019 21:37:42 -0800
> For tcindex filter, it is too late to initialize the
> net pointer in tcf_exts_validate(), as tcf_exts_get_net()
> requires a non-NULL net pointer. We can just move its
> initialization into tcf_exts_init(), which just requires
> an additional parameter.
>
> This makes the code in tcindex_alloc_perfect_hash()
> prettier.
>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks Cong.
^ permalink raw reply
* Re: [PATCH] Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255
From: Kalash Nainwal @ 2019-02-22 23:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20190222.152226.1935918245873046733.davem@davemloft.net>
On Fri, Feb 22, 2019 at 3:22 PM David Miller <davem@davemloft.net> wrote:
>
> From: Kalash Nainwal <kalash@arista.com>
> Date: Wed, 20 Feb 2019 16:23:04 -0800
>
> > Set rtm_table to RT_TABLE_COMPAT for ipv6 for tables > 255 to
> > keep legacy software happy. This is similar to what was done for
> > ipv4 in commit 709772e6e065 ("net: Fix routing tables with
> > id > 255 for legacy software").
> >
> > Signed-off-by: Kalash Nainwal <kalash@arista.com>
>
> Applied and queued up for -stable.
Thanks Dave.
^ permalink raw reply
* Re: [PATCH net-next v4 0/2] net: phy: at803x: Update delays for RGMII modes
From: David Miller @ 2019-02-22 23:32 UTC (permalink / raw)
To: vkoul
Cc: linux-arm-msm, bjorn.andersson, netdev, niklas.cassel, andrew,
f.fainelli, nsekhar, peter.ujfalusi, marc.w.gonzalez
In-Reply-To: <20190221102315.12946-1-vkoul@kernel.org>
From: Vinod Koul <vkoul@kernel.org>
Date: Thu, 21 Feb 2019 15:53:13 +0530
> Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable
> phy delay for RGMII mode") caused regression on am335x-evmsk board.
> This board expects the Phy delay to be enabled but specified RGMII mode
> which refers to delays being disabled. So fix this by disabling delay only
> for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes.
>
> While at it, as pointed by Dave, don't inline the helpers.
>
> [1]: https://www.spinics.net/lists/netdev/msg550749.html
>
> Changes in v4:
> - fix log & comments nbased on Marc's feedback
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] isdn_common: Mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2019-02-22 23:34 UTC (permalink / raw)
To: David Miller; +Cc: isdn, netdev, linux-kernel, keescook
In-Reply-To: <20190222.115023.1085574898692218507.davem@davemloft.net>
On 2/22/19 1:50 PM, David Miller wrote:
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Applied.
>
Thanks, Dave.
I wonder if you can take this one too:
https://lore.kernel.org/lkml/20190129180612.GA28650@embeddedor/
--
Gustavo
^ permalink raw reply
* Re: [PATCH] mdio_bus: Fix use-after-free on device_register fails
From: David Miller @ 2019-02-22 23:35 UTC (permalink / raw)
To: yuehaibing; +Cc: andrew, f.fainelli, hkallweit1, linux-kernel, netdev
In-Reply-To: <20190221144201.11488-1-yuehaibing@huawei.com>
From: Yue Haibing <yuehaibing@huawei.com>
Date: Thu, 21 Feb 2019 22:42:01 +0800
> From: YueHaibing <yuehaibing@huawei.com>
>
> KASAN has found use-after-free in fixed_mdio_bus_init,
> commit 0c692d07842a ("drivers/net/phy/mdio_bus.c: call
> put_device on device_register() failure") call put_device()
> while device_register() fails,give up the last reference
> to the device and allow mdiobus_release to be executed
> ,kfreeing the bus. However in most drives, mdiobus_free
> be called to free the bus while mdiobus_register fails.
> use-after-free occurs when access bus again, this patch
> revert it to let mdiobus_free free the bus.
>
> KASAN report details as below:
...
> Fixes: 0c692d07842a ("drivers/net/phy/mdio_bus.c: call put_device on device_register() failure")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH bpf-next 0/4] bpf: per program stats
From: Alexei Starovoitov @ 2019-02-22 23:36 UTC (permalink / raw)
To: davem; +Cc: daniel, netdev, bpf, kernel-team
Introduce per program stats to monitor the usage BPF
Alexei Starovoitov (4):
bpf: enable program stats
bpf: expose program stats via bpf_prog_info
tools/bpf: sync bpf.h into tools
tools/bpftool: recognize bpf_prog_info runtime and runcnt
include/linux/bpf.h | 7 ++++
include/linux/filter.h | 16 ++++++++-
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/core.c | 13 +++++++
kernel/bpf/syscall.c | 29 ++++++++++++++--
kernel/bpf/verifier.c | 5 +++
kernel/sysctl.c | 34 +++++++++++++++++++
.../bpftool/Documentation/bpftool-prog.rst | 4 ++-
tools/bpf/bpftool/prog.c | 7 ++++
tools/include/uapi/linux/bpf.h | 2 ++
10 files changed, 115 insertions(+), 4 deletions(-)
--
2.20.0
^ 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