* [PATCH 2/2][ethtool] ethtool: implement support for Energy Detect Power Down
From: Alexandru Ardelean @ 2019-09-19 10:08 UTC (permalink / raw)
To: netdev; +Cc: linville, andrew, f.fainelli, Alexandru Ardelean
In-Reply-To: <20190919100833.6208-1-alexandru.ardelean@analog.com>
This change adds control for enabling/disabling Energy Detect Power Down
mode, as well as configuring wake-up intervals for TX pulses, via the new
ETHTOOL_PHY_EDPD control added in PHY tunable, in the kernel.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
ethtool.8.in | 28 +++++++++++++++++
ethtool.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 111 insertions(+), 4 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index cd3be91..609a05a 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -362,11 +362,17 @@ ethtool \- query or control network driver and hardware settings
.A1 on off
.BN msecs
.RB ]
+.RB [
+.B energy\-detect\-power\-down
+.A1 on off
+.BN msecs
+.RB ]
.HP
.B ethtool \-\-get\-phy\-tunable
.I devname
.RB [ downshift ]
.RB [ fast-link-down ]
+.RB [ energy-detect-power-down ]
.HP
.B ethtool \-\-reset
.I devname
@@ -1100,6 +1106,24 @@ lB l.
Sets the period after which the link is reported as down. Note that the PHY may choose
the closest supported value. Only on reading back the tunable do you get the actual value.
.TE
+.TP
+.A2 energy-detect-power-down on off
+Specifies whether Energy Detect Power Down (EDPD) should be enabled (if supported).
+This will put the RX and TX circuit blocks into a low power mode, and the PHY will
+wake up periodically to send link pulses to avoid any lock-up situation with a peer
+PHY that may also have EDPD enabled. By default, this setting will also enable the
+periodic transmission of TX pulses.
+.TS
+nokeep;
+lB l.
+.BI msecs \ N
+ Some PHYs support configuration of the wake-up interval to send TX pulses.
+ This setting allows the control of this interval, and 0 disables TX pulses
+ if the PHY supports this. Disabling TX pulses can create a lock-up situation
+ where neither of the PHYs wakes the other one. If unspecified the default
+ value (in milliseconds) will be used by the PHY.
+.TE
+.TP
.PD
.RE
.TP
@@ -1122,6 +1146,10 @@ Some PHYs support a Fast Link Down Feature and may allow configuration of the de
before a broken link is reported as being down.
Gets the PHY Fast Link Down status / period.
+.TP
+.B energy\-detect\-power\-down
+Gets the current configured setting for Energy Detect Power Down (if supported).
+
.RE
.TP
.B \-\-reset
diff --git a/ethtool.c b/ethtool.c
index c0e2903..45b5478 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4897,6 +4897,30 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
else
fprintf(stdout, "Fast Link Down enabled, %d msecs\n",
cont.msecs);
+ } else if (!strcmp(argp[0], "energy-detect-power-down")) {
+ struct {
+ struct ethtool_tunable ds;
+ u16 msecs;
+ } cont;
+
+ cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
+ cont.ds.id = ETHTOOL_PHY_EDPD;
+ cont.ds.type_id = ETHTOOL_TUNABLE_U16;
+ cont.ds.len = 2;
+ if (send_ioctl(ctx, &cont.ds) < 0) {
+ perror("Cannot Get PHY Energy Detect Power Down value");
+ return 87;
+ }
+
+ if (cont.msecs == ETHTOOL_PHY_EDPD_DISABLE)
+ fprintf(stdout, "Energy Detect Power Down: disabled\n");
+ else if (cont.msecs == ETHTOOL_PHY_EDPD_NO_TX)
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX disabled\n");
+ else
+ fprintf(stdout,
+ "Energy Detect Power Down: enabled, TX %u msecs\n",
+ cont.msecs);
} else {
exit_bad_args();
}
@@ -5018,7 +5042,8 @@ static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
return 1;
}
-static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+static int parse_named_uint(struct cmd_context *ctx, const char *name,
+ void *val, enum tunable_type_id type_id)
{
if (ctx->argc < 2)
return 0;
@@ -5026,7 +5051,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
if (strcmp(*ctx->argp, name))
return 0;
- *val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ switch (type_id) {
+ case ETHTOOL_TUNABLE_U8:
+ *(u8 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xff);
+ break;
+ case ETHTOOL_TUNABLE_U16:
+ *(u16 *)val = get_uint_range(*(ctx->argp + 1), 0, 0xffff);
+ break;
+ default:
+ return 0;
+ }
ctx->argc -= 2;
ctx->argp += 2;
@@ -5034,6 +5068,16 @@ static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
return 1;
}
+static int parse_named_u8(struct cmd_context *ctx, const char *name, u8 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U8);
+}
+
+static int parse_named_u16(struct cmd_context *ctx, const char *name, u16 *val)
+{
+ return parse_named_uint(ctx, name, val, ETHTOOL_TUNABLE_U16);
+}
+
static int do_set_phy_tunable(struct cmd_context *ctx)
{
int err = 0;
@@ -5041,6 +5085,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
u8 ds_changed = 0, ds_has_cnt = 0, ds_enable = 0;
u8 fld_changed = 0, fld_enable = 0;
u8 fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_ON;
+ u8 edpd_changed = 0, edpd_enable = 0;
+ u16 edpd_tx_interval = ETHTOOL_PHY_EDPD_DFLT_TX_MSECS;
/* Parse arguments */
if (parse_named_bool(ctx, "downshift", &ds_enable)) {
@@ -5050,6 +5096,11 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_changed = 1;
if (fld_enable)
parse_named_u8(ctx, "msecs", &fld_msecs);
+ } else if (parse_named_bool(ctx, "energy-detect-power-down",
+ &edpd_enable)) {
+ edpd_changed = 1;
+ if (edpd_enable)
+ parse_named_u16(ctx, "msecs", &edpd_tx_interval);
} else {
exit_bad_args();
}
@@ -5074,6 +5125,16 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
fld_msecs = ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
else if (fld_msecs == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
exit_bad_args();
+ } else if (edpd_changed) {
+ if (!edpd_enable)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_DISABLE;
+ else if (edpd_tx_interval == 0)
+ edpd_tx_interval = ETHTOOL_PHY_EDPD_NO_TX;
+ else if (edpd_tx_interval > ETHTOOL_PHY_EDPD_NO_TX) {
+ fprintf(stderr, "'msecs' max value is %d.\n",
+ (ETHTOOL_PHY_EDPD_NO_TX - 1));
+ exit_bad_args();
+ }
}
/* Do it */
@@ -5109,6 +5170,22 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
perror("Cannot Set PHY Fast Link Down value");
err = 87;
}
+ } else if (edpd_changed) {
+ struct {
+ struct ethtool_tunable fld;
+ u16 msecs;
+ } cont;
+
+ cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
+ cont.fld.id = ETHTOOL_PHY_EDPD;
+ cont.fld.type_id = ETHTOOL_TUNABLE_U16;
+ cont.fld.len = 2;
+ cont.msecs = edpd_tx_interval;
+ err = send_ioctl(ctx, &cont.fld);
+ if (err < 0) {
+ perror("Cannot Set PHY Energy Detect Power Down");
+ err = 87;
+ }
}
return err;
@@ -5361,10 +5438,12 @@ static const struct option {
" [ tx-timer %d ]\n"},
{ "--set-phy-tunable", 1, do_set_phy_tunable, "Set PHY tunable",
" [ downshift on|off [count N] ]\n"
- " [ fast-link-down on|off [msecs N] ]\n"},
+ " [ fast-link-down on|off [msecs N] ]\n"
+ " [ energy-detect-power-down on|off [msecs N] ]\n"},
{ "--get-phy-tunable", 1, do_get_phy_tunable, "Get PHY tunable",
" [ downshift ]\n"
- " [ fast-link-down ]\n"},
+ " [ fast-link-down ]\n"
+ " [ energy-detect-power-down ]\n"},
{ "--reset", 1, do_reset, "Reset components",
" [ flags %x ]\n"
" [ mgmt ]\n"
--
2.20.1
^ permalink raw reply related
* [PATCH 1/2][ethtool] ethtool: sync ethtool-copy.h: adds support for EDPD
From: Alexandru Ardelean @ 2019-09-19 10:08 UTC (permalink / raw)
To: netdev; +Cc: linville, andrew, f.fainelli, Alexandru Ardelean
This change syncs the `ethtool-copy.h` file with Linux net-next to add
support for Energy Detect Powerdown control via phy tunable.
net-next commit/sync-point:
commit 1bab8d4c488be22d57f9dd09968c90a0ddc413bf
Merge: 990925fad5c2 00b368502d18
Author: David S. Miller <davem@davemloft.net>
Date: Tue Sep 17 23:51:10 2019 +0200
Merge ra.kernel.org:/pub/scm/linux/kernel/git/netdev/net
Pull in bug fixes from 'net' tree for the merge window.
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
ethtool-copy.h | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index ad16e8f..9afd2e6 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -257,10 +257,32 @@ struct ethtool_tunable {
#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0
#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff
+/* Energy Detect Power Down (EDPD) is a feature supported by some PHYs, where
+ * the PHY's RX & TX blocks are put into a low-power mode when there is no
+ * link detected (typically cable is un-plugged). For RX, only a minimal
+ * link-detection is available, and for TX the PHY wakes up to send link pulses
+ * to avoid any lock-ups in case the peer PHY may also be running in EDPD mode.
+ *
+ * Some PHYs may support configuration of the wake-up interval for TX pulses,
+ * and some PHYs may support only disabling TX pulses entirely. For the latter
+ * a special value is required (ETHTOOL_PHY_EDPD_NO_TX) so that this can be
+ * configured from userspace (should the user want it).
+ *
+ * The interval units for TX wake-up are in milliseconds, since this should
+ * cover a reasonable range of intervals:
+ * - from 1 millisecond, which does not sound like much of a power-saver
+ * - to ~65 seconds which is quite a lot to wait for a link to come up when
+ * plugging a cable
+ */
+#define ETHTOOL_PHY_EDPD_DFLT_TX_MSECS 0xffff
+#define ETHTOOL_PHY_EDPD_NO_TX 0xfffe
+#define ETHTOOL_PHY_EDPD_DISABLE 0
+
enum phy_tunable_id {
ETHTOOL_PHY_ID_UNSPEC,
ETHTOOL_PHY_DOWNSHIFT,
ETHTOOL_PHY_FAST_LINK_DOWN,
+ ETHTOOL_PHY_EDPD,
/*
* Add your fresh new phy tunable attribute above and remember to update
* phy_tunable_strings[] in net/core/ethtool.c
@@ -1481,8 +1503,8 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65,
ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
- ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
- ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
+ ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
+ ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
/* must be last entry */
__ETHTOOL_LINK_MODE_MASK_NBITS
@@ -1712,8 +1734,8 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
#define ETH_MODULE_SFF_8436 0x4
#define ETH_MODULE_SFF_8436_LEN 256
-#define ETH_MODULE_SFF_8636_MAX_LEN 640
-#define ETH_MODULE_SFF_8436_MAX_LEN 640
+#define ETH_MODULE_SFF_8636_MAX_LEN 640
+#define ETH_MODULE_SFF_8436_MAX_LEN 640
/* Reset flags */
/* The reset() operation must clear the flags for the components which
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/4] seccomp: add SECCOMP_RET_USER_NOTIF_ALLOW
From: Christian Brauner @ 2019-09-19 6:53 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, luto, jannh, wad, shuah, ast, daniel, kafai,
songliubraving, yhs, linux-kernel, linux-kselftest, netdev, bpf,
Tyler Hicks
In-Reply-To: <20190918180712.GG3835@cisco>
On Wed, Sep 18, 2019 at 12:07:12PM -0600, Tycho Andersen wrote:
> On Wed, Sep 18, 2019 at 10:30:00AM -0700, Kees Cook wrote:
> > On Wed, Sep 18, 2019 at 10:48:30AM +0200, Christian Brauner wrote:
> > > This allows the seccomp notifier to continue a syscall. A positive
> > > discussion about this feature was triggered by a post to the
> > > ksummit-discuss mailing list (cf. [3]) and took place during KSummit
> > > (cf. [1]) and again at the containers/checkpoint-restore
> > > micro-conference at Linux Plumbers.
> > >
> > > Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> > > which enables a process (watchee) to retrieve an fd for its seccomp
> > > filter. This fd can then be handed to another (usually more privileged)
> > > process (watcher). The watcher will then be able to receive seccomp
> > > messages about the syscalls having been performed by the watchee.
> > >
> > > This feature is heavily used in some userspace workloads. For example,
> > > it is currently used to intercept mknod() syscalls in user namespaces
> > > aka in containers.
> > > The mknod() syscall can be easily filtered based on dev_t. This allows
> > > us to only intercept a very specific subset of mknod() syscalls.
> > > Furthermore, mknod() is not possible in user namespaces toto coelo and
> > > so intercepting and denying syscalls that are not in the whitelist on
> > > accident is not a big deal. The watchee won't notice a difference.
> > >
> > > In contrast to mknod(), a lot of other syscall we intercept (e.g.
> > > setxattr()) cannot be easily filtered like mknod() because they have
> > > pointer arguments. Additionally, some of them might actually succeed in
> > > user namespaces (e.g. setxattr() for all "user.*" xattrs). Since we
> > > currently cannot tell seccomp to continue from a user notifier we are
> > > stuck with performing all of the syscalls in lieu of the container. This
> > > is a huge security liability since it is extremely difficult to
> > > correctly assume all of the necessary privileges of the calling task
> > > such that the syscall can be successfully emulated without escaping
> > > other additional security restrictions (think missing CAP_MKNOD for
> > > mknod(), or MS_NODEV on a filesystem etc.). This can be solved by
> > > telling seccomp to resume the syscall.
> > >
> > > One thing that came up in the discussion was the problem that another
> > > thread could change the memory after userspace has decided to let the
> > > syscall continue which is a well known TOCTOU with seccomp which is
> > > present in other ways already.
> > > The discussion showed that this feature is already very useful for any
> > > syscall without pointer arguments. For any accidentally intercepted
> > > non-pointer syscall it is safe to continue.
> > > For syscalls with pointer arguments there is a race but for any cautious
> > > userspace and the main usec cases the race doesn't matter. The notifier
> > > is intended to be used in a scenario where a more privileged watcher
> > > supervises the syscalls of lesser privileged watchee to allow it to get
> > > around kernel-enforced limitations by performing the syscall for it
> > > whenever deemed save by the watcher. Hence, if a user tricks the watcher
> > > into allowing a syscall they will either get a deny based on
> > > kernel-enforced restrictions later or they will have changed the
> > > arguments in such a way that they manage to perform a syscall with
> > > arguments that they would've been allowed to do anyway.
> > > In general, it is good to point out again, that the notifier fd was not
> > > intended to allow userspace to implement a security policy but rather to
> > > work around kernel security mechanisms in cases where the watcher knows
> > > that a given action is safe to perform.
> > >
> > > /* References */
> > > [1]: https://linuxplumbersconf.org/event/4/contributions/560
> > > [2]: https://linuxplumbersconf.org/event/4/contributions/477
> > > [3]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
> > > [4]: commit 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> > >
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: Will Drewry <wad@chromium.org>
> > > Cc: Tycho Andersen <tycho@tycho.ws>
> > > CC: Tyler Hicks <tyhicks@canonical.com>
> > > Cc: Jann Horn <jannh@google.com>
> > > ---
> > > include/uapi/linux/seccomp.h | 2 ++
> > > kernel/seccomp.c | 24 ++++++++++++++++++++----
> > > 2 files changed, 22 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > > index 90734aa5aa36..2c23b9aa6383 100644
> > > --- a/include/uapi/linux/seccomp.h
> > > +++ b/include/uapi/linux/seccomp.h
> > > @@ -76,6 +76,8 @@ struct seccomp_notif {
> > > struct seccomp_data data;
> > > };
> > >
> > > +#define SECCOMP_RET_USER_NOTIF_ALLOW 0x00000001
> >
> > nit: I'd like to avoid confusion here about what "family" these flags
> > belong to. "SECCOMP_RET_..." is used for the cBPF filter return action
> > value, so let's instead call this:
> >
> > #define SECCOMP_USER_NOTIF_CONTINUE BIT(0)
>
> +1, I was thinking maybe even SECCOMP_USER_NOTIF_FLAG_CONTINUE.
I'll flip a coin between yours and Kees suggestion. :)
>
> But the whole series (minus the patch that already exists) looks good
> to me if we make this change:
>
> Reviewed-by: Tycho Andersen <tycho@tycho.ws>
Thanks for the review! :)
Christian
^ permalink raw reply
* Re: [PATCH 1/4] seccomp: add SECCOMP_RET_USER_NOTIF_ALLOW
From: Christian Brauner @ 2019-09-19 6:53 UTC (permalink / raw)
To: Kees Cook
Cc: luto, jannh, wad, shuah, ast, daniel, kafai, songliubraving, yhs,
linux-kernel, linux-kselftest, netdev, bpf, Tycho Andersen,
Tyler Hicks
In-Reply-To: <201909181018.E3CEC9A81@keescook>
On Wed, Sep 18, 2019 at 10:30:00AM -0700, Kees Cook wrote:
> On Wed, Sep 18, 2019 at 10:48:30AM +0200, Christian Brauner wrote:
> > This allows the seccomp notifier to continue a syscall. A positive
> > discussion about this feature was triggered by a post to the
> > ksummit-discuss mailing list (cf. [3]) and took place during KSummit
> > (cf. [1]) and again at the containers/checkpoint-restore
> > micro-conference at Linux Plumbers.
> >
> > Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> > which enables a process (watchee) to retrieve an fd for its seccomp
> > filter. This fd can then be handed to another (usually more privileged)
> > process (watcher). The watcher will then be able to receive seccomp
> > messages about the syscalls having been performed by the watchee.
> >
> > This feature is heavily used in some userspace workloads. For example,
> > it is currently used to intercept mknod() syscalls in user namespaces
> > aka in containers.
> > The mknod() syscall can be easily filtered based on dev_t. This allows
> > us to only intercept a very specific subset of mknod() syscalls.
> > Furthermore, mknod() is not possible in user namespaces toto coelo and
> > so intercepting and denying syscalls that are not in the whitelist on
> > accident is not a big deal. The watchee won't notice a difference.
> >
> > In contrast to mknod(), a lot of other syscall we intercept (e.g.
> > setxattr()) cannot be easily filtered like mknod() because they have
> > pointer arguments. Additionally, some of them might actually succeed in
> > user namespaces (e.g. setxattr() for all "user.*" xattrs). Since we
> > currently cannot tell seccomp to continue from a user notifier we are
> > stuck with performing all of the syscalls in lieu of the container. This
> > is a huge security liability since it is extremely difficult to
> > correctly assume all of the necessary privileges of the calling task
> > such that the syscall can be successfully emulated without escaping
> > other additional security restrictions (think missing CAP_MKNOD for
> > mknod(), or MS_NODEV on a filesystem etc.). This can be solved by
> > telling seccomp to resume the syscall.
> >
> > One thing that came up in the discussion was the problem that another
> > thread could change the memory after userspace has decided to let the
> > syscall continue which is a well known TOCTOU with seccomp which is
> > present in other ways already.
> > The discussion showed that this feature is already very useful for any
> > syscall without pointer arguments. For any accidentally intercepted
> > non-pointer syscall it is safe to continue.
> > For syscalls with pointer arguments there is a race but for any cautious
> > userspace and the main usec cases the race doesn't matter. The notifier
> > is intended to be used in a scenario where a more privileged watcher
> > supervises the syscalls of lesser privileged watchee to allow it to get
> > around kernel-enforced limitations by performing the syscall for it
> > whenever deemed save by the watcher. Hence, if a user tricks the watcher
> > into allowing a syscall they will either get a deny based on
> > kernel-enforced restrictions later or they will have changed the
> > arguments in such a way that they manage to perform a syscall with
> > arguments that they would've been allowed to do anyway.
> > In general, it is good to point out again, that the notifier fd was not
> > intended to allow userspace to implement a security policy but rather to
> > work around kernel security mechanisms in cases where the watcher knows
> > that a given action is safe to perform.
> >
> > /* References */
> > [1]: https://linuxplumbersconf.org/event/4/contributions/560
> > [2]: https://linuxplumbersconf.org/event/4/contributions/477
> > [3]: https://lore.kernel.org/r/20190719093538.dhyopljyr5ns33qx@brauner.io
> > [4]: commit 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> >
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: Tycho Andersen <tycho@tycho.ws>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > Cc: Jann Horn <jannh@google.com>
> > ---
> > include/uapi/linux/seccomp.h | 2 ++
> > kernel/seccomp.c | 24 ++++++++++++++++++++----
> > 2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > index 90734aa5aa36..2c23b9aa6383 100644
> > --- a/include/uapi/linux/seccomp.h
> > +++ b/include/uapi/linux/seccomp.h
> > @@ -76,6 +76,8 @@ struct seccomp_notif {
> > struct seccomp_data data;
> > };
> >
> > +#define SECCOMP_RET_USER_NOTIF_ALLOW 0x00000001
>
> nit: I'd like to avoid confusion here about what "family" these flags
> belong to. "SECCOMP_RET_..." is used for the cBPF filter return action
> value, so let's instead call this:
>
> #define SECCOMP_USER_NOTIF_CONTINUE BIT(0)
Ack.
>
> I'm thinking of "continue" as slightly different from "allow", in the
> sense that I'd like to hint that this doesn't mean arguments could have
> been reliably "filtered" via user notification.
Good point.
>
> And at the same time, please add a giant comment about this in the
> header that details the purpose ("check if I should do something on
> behalf of the process") and not "is this safe to allow?", due to the
> argument parsing ToCToU.
Yeah, I'll copy parts of what I described in the commit message down
into the code.
>
> > -static void seccomp_do_user_notification(int this_syscall,
> > +static bool seccomp_do_user_notification(int this_syscall,
>
> I'd prefer this stay an "int", just to keep it similar to the other
> functions that are checked in __seccomp_filter().
Ack.
>
> > + /* perform syscall */
>
> nit: expand this commit to something like "Userspace requests we
> continue and perform syscall".
Ack.
>
> > + if (flags & SECCOMP_RET_USER_NOTIF_ALLOW)
> > + return false;
>
> return 0;
>
> > +
> > syscall_set_return_value(current, task_pt_regs(current),
> > err, ret);
> > + return true;
>
> return -1;
>
> (This makes it look more like a "skip on failure")
Ack.
>
> > + if (resp.flags & ~SECCOMP_RET_USER_NOTIF_ALLOW)
> > + return -EINVAL;
> > +
> > + if ((resp.flags & SECCOMP_RET_USER_NOTIF_ALLOW) &&
> > + (resp.error || resp.val))
> > return -EINVAL;
>
> Ah yeah, good idea.
>
> Beyond these nits, yes, looks good and should help the usability of this
> feature. Thanks for getting it written and tested!
Will rework and resend!
Thanks for the review! :)
Christian
^ permalink raw reply
* Re: [Patch net] net_sched: add max len check for TCA_KIND
From: Jiri Pirko @ 2019-09-19 6:40 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, syzbot+618aacd49e8c8b8486bd, David Ahern,
Jamal Hadi Salim
In-Reply-To: <20190918232412.16718-1-xiyou.wangcong@gmail.com>
Thu, Sep 19, 2019 at 01:24:12AM CEST, xiyou.wangcong@gmail.com wrote:
>The TCA_KIND attribute is of NLA_STRING which does not check
>the NUL char. KMSAN reported an uninit-value of TCA_KIND which
>is likely caused by the lack of NUL.
>
>Change it to NLA_NUL_STRING and add a max len too.
>
>Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* Re: [PATCH v4 1/3] kernel/notifier.c: intercepting duplicate registrations to avoid infinite loops
From: Greg KH @ 2019-09-19 6:36 UTC (permalink / raw)
To: Xiaoming Ni
Cc: akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan, bfields,
chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey, paulmck,
semen.protsenko, stern, tglx, trond.myklebust, viresh.kumar,
stable, dylix.dailei, yuehaibing, linux-kernel, linux-nfs, netdev
In-Reply-To: <1568861888-34045-2-git-send-email-nixiaoming@huawei.com>
On Thu, Sep 19, 2019 at 10:58:06AM +0800, Xiaoming Ni wrote:
> Registering the same notifier to a hook repeatedly can cause the hook
> list to form a ring or lose other members of the list.
>
> case1: An infinite loop in notifier_chain_register() can cause soft lockup
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test2);
>
> case2: An infinite loop in notifier_chain_register() can cause soft lockup
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_call_chain(&test_notifier_list, 0, NULL);
>
> case3: lose other hook test2
> atomic_notifier_chain_register(&test_notifier_list, &test1);
> atomic_notifier_chain_register(&test_notifier_list, &test2);
> atomic_notifier_chain_register(&test_notifier_list, &test1);
>
> case4: Unregister returns 0, but the hook is still in the linked list,
> and it is not really registered. If you call notifier_call_chain
> after ko is unloaded, it will trigger oops.
>
> If the system is configured with softlockup_panic and the same
> hook is repeatedly registered on the panic_notifier_list, it
> will cause a loop panic.
>
> Add a check in notifier_chain_register(),
> Intercepting duplicate registrations to avoid infinite loops
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> Reviewed-by: Vasily Averin <vvs@virtuozzo.com>
> ---
> kernel/notifier.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
Same thing goes for all of the patches in this series.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH net] net: dsa: sja1105: Add dependency for NET_DSA_SJA1105_TAS
From: Vladimir Oltean @ 2019-09-19 6:25 UTC (permalink / raw)
To: Mao Wenan
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
lkml, netdev, kernel-janitors
In-Reply-To: <20190919063819.164826-1-maowenan@huawei.com>
On Thu, 19 Sep 2019 at 09:22, Mao Wenan <maowenan@huawei.com> wrote:
>
> If CONFIG_NET_DSA_SJA1105_TAS=y and CONFIG_NET_SCH_TAPRIO=n,
> below error can be found:
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_setup_tc_taprio':
> sja1105_tas.c:(.text+0x318): undefined reference to `taprio_offload_free'
> sja1105_tas.c:(.text+0x590): undefined reference to `taprio_offload_get'
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_tas_teardown':
> sja1105_tas.c:(.text+0x610): undefined reference to `taprio_offload_free'
> make: *** [vmlinux] Error 1
>
> sja1105_tas needs tc-taprio, so this patch add the dependency for it.
>
> Fixes: 317ab5b86c8e ("net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> drivers/net/dsa/sja1105/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
> index 55424f3..f40b248 100644
> --- a/drivers/net/dsa/sja1105/Kconfig
> +++ b/drivers/net/dsa/sja1105/Kconfig
> @@ -27,6 +27,7 @@ config NET_DSA_SJA1105_PTP
> config NET_DSA_SJA1105_TAS
> bool "Support for the Time-Aware Scheduler on NXP SJA1105"
> depends on NET_DSA_SJA1105
> + depends on NET_SCH_TAPRIO
> help
> This enables support for the TTEthernet-based egress scheduling
> engine in the SJA1105 DSA driver, which is controlled using a
> --
> 2.7.4
>
Thanks!
-Vladimir
^ permalink raw reply
* [PATCH net] net: dsa: sja1105: Add dependency for NET_DSA_SJA1105_TAS
From: Mao Wenan @ 2019-09-19 6:38 UTC (permalink / raw)
To: olteanv, andrew, vivien.didelot, f.fainelli, davem
Cc: linux-kernel, netdev, kernel-janitors, Mao Wenan
If CONFIG_NET_DSA_SJA1105_TAS=y and CONFIG_NET_SCH_TAPRIO=n,
below error can be found:
drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_setup_tc_taprio':
sja1105_tas.c:(.text+0x318): undefined reference to `taprio_offload_free'
sja1105_tas.c:(.text+0x590): undefined reference to `taprio_offload_get'
drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_tas_teardown':
sja1105_tas.c:(.text+0x610): undefined reference to `taprio_offload_free'
make: *** [vmlinux] Error 1
sja1105_tas needs tc-taprio, so this patch add the dependency for it.
Fixes: 317ab5b86c8e ("net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/net/dsa/sja1105/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
index 55424f3..f40b248 100644
--- a/drivers/net/dsa/sja1105/Kconfig
+++ b/drivers/net/dsa/sja1105/Kconfig
@@ -27,6 +27,7 @@ config NET_DSA_SJA1105_PTP
config NET_DSA_SJA1105_TAS
bool "Support for the Time-Aware Scheduler on NXP SJA1105"
depends on NET_DSA_SJA1105
+ depends on NET_SCH_TAPRIO
help
This enables support for the TTEthernet-based egress scheduling
engine in the SJA1105 DSA driver, which is controlled using a
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] devlink: add devlink notification for recovery
From: Jiri Pirko @ 2019-09-19 6:11 UTC (permalink / raw)
To: Sheetal Tigadoli
Cc: Jiri Pirko, David S. Miller, Ray Jui, Vikram Prakash,
linux-kernel, netdev, BCM Kernel Feedback, Vikas Gupta
In-Reply-To: <1568832741-20850-1-git-send-email-sheetal.tigadoli@broadcom.com>
Wed, Sep 18, 2019 at 08:52:21PM CEST, sheetal.tigadoli@broadcom.com wrote:
>From: Vikas Gupta <vikas.gupta@broadcom.com>
>
>Add a devlink notification for reporter recovery
>
>Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
>Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli@broadcom.com>
>---
> net/core/devlink.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index e48680e..42909fb 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4730,6 +4730,28 @@ struct devlink_health_reporter *
> }
> EXPORT_SYMBOL_GPL(devlink_health_reporter_state_update);
>
>+static void __devlink_recover_notify(struct devlink *devlink,
>+ enum devlink_command cmd)
>+{
>+ struct sk_buff *msg;
>+ int err;
>+
>+ WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
>+
>+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+ if (!msg)
>+ return;
>+
>+ err = devlink_nl_fill(msg, devlink, cmd, 0, 0, 0);
>+ if (err) {
>+ nlmsg_free(msg);
>+ return;
>+ }
>+
>+ genlmsg_multicast_netns(&devlink_nl_family, devlink_net(devlink),
>+ msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
>+}
>+
> static int
> devlink_health_reporter_recover(struct devlink_health_reporter *reporter,
> void *priv_ctx)
>@@ -4747,6 +4769,9 @@ struct devlink_health_reporter *
> reporter->health_state = DEVLINK_HEALTH_REPORTER_STATE_HEALTHY;
> reporter->last_recovery_ts = jiffies;
>
>+ __devlink_recover_notify(reporter->devlink,
>+ DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
>+
> return 0;
> }
To follow the rest of the code The notification should be done upon
any reported change, using devlink_nl_health_reporter_fill() to prepare
the message.
Also, this is net-next patch net-next is closed now.
>
^ permalink raw reply
* Re: INFO: task hung in cancel_delayed_work_sync
From: Eric Biggers @ 2019-09-19 5:50 UTC (permalink / raw)
To: Steffen Klassert
Cc: syzbot, ast, aviadye, borisp, bpf, daniel, davejwatson, davem,
ilyal, jakub.kicinski, john.fastabend, kafai, linux-crypto,
linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs
In-Reply-To: <20190919053620.GG2879@gauss3.secunet.de>
On Thu, Sep 19, 2019 at 07:36:20AM +0200, Steffen Klassert wrote:
> On Wed, Sep 18, 2019 at 10:15:45PM -0700, Eric Biggers wrote:
> >
> > Reproducer involves pcrypt, so probably the pcrypt deadlock again...
> > https://lkml.kernel.org/linux-crypto/20190817054743.GE8209@sol.localdomain/
>
> I'll submit the patch I proposed here in case noone has a better idea
> how to fix this now:
>
> https://lkml.kernel.org/linux-crypto/20190821063704.GM2879@gauss3.secunet.de/
>
> The original patch is from you, I did some modifications to forbid
> pcrypt if an underlying algorithm needs a fallback.
>
> May I leave your 'Signed off' on this patch, or just
> quote that the initial version is from you?
>
Keeping my Signed-off-by is fine, but please leave a note about what you
changed, like:
Signed-off-by: Eric Biggers <ebiggers@google.com>
[SK: also require that the underlying algorithm doesn't need a fallback]
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
Also, a nit: in the commit message,
> Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> underlying ->cra_driver_name and if an underlying algorithm needs a
> fallback.
... the word "and" should be "or".
- Eric
^ permalink raw reply
* Re: INFO: task hung in cancel_delayed_work_sync
From: Steffen Klassert @ 2019-09-19 5:36 UTC (permalink / raw)
To: syzbot, ast, aviadye, borisp, bpf, daniel, davejwatson, davem,
ilyal, jakub.kicinski, john.fastabend, kafai, linux-crypto,
linux-kernel, netdev, songliubraving, syzkaller-bugs, yhs
In-Reply-To: <20190919051545.GB666@sol.localdomain>
On Wed, Sep 18, 2019 at 10:15:45PM -0700, Eric Biggers wrote:
>
> Reproducer involves pcrypt, so probably the pcrypt deadlock again...
> https://lkml.kernel.org/linux-crypto/20190817054743.GE8209@sol.localdomain/
I'll submit the patch I proposed here in case noone has a better idea
how to fix this now:
https://lkml.kernel.org/linux-crypto/20190821063704.GM2879@gauss3.secunet.de/
The original patch is from you, I did some modifications to forbid
pcrypt if an underlying algorithm needs a fallback.
May I leave your 'Signed off' on this patch, or just
quote that the initial version is from you?
^ permalink raw reply
* Re: INFO: task hung in cancel_delayed_work_sync
From: Eric Biggers @ 2019-09-19 5:15 UTC (permalink / raw)
To: steffen.klassert, syzbot
Cc: ast, aviadye, borisp, bpf, daniel, davejwatson, davem, ilyal,
jakub.kicinski, john.fastabend, kafai, linux-crypto, linux-kernel,
netdev, songliubraving, syzkaller-bugs, yhs
In-Reply-To: <0000000000001348750592a8ef50@google.com>
On Mon, Sep 16, 2019 at 03:19:06AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: f4b752a6 mlx4: fix spelling mistake "veify" -> "verify"
> git tree: net
> console output: https://syzkaller.appspot.com/x/log.txt?x=1183c7fa600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=b89bb446a3faaba4
> dashboard link: https://syzkaller.appspot.com/bug?extid=f39ab8494f6015e62360
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14426d85600000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=110c1af1600000
>
> The bug was bisected to:
>
> commit 3c4d7559159bfe1e3b94df3a657b2cda3a34e218
> Author: Dave Watson <davejwatson@fb.com>
> Date: Wed Jun 14 18:37:39 2017 +0000
>
> tls: kernel TLS support
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=144a4ffa600000
> final crash: https://syzkaller.appspot.com/x/report.txt?x=164a4ffa600000
> console output: https://syzkaller.appspot.com/x/log.txt?x=124a4ffa600000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+f39ab8494f6015e62360@syzkaller.appspotmail.com
> Fixes: 3c4d7559159b ("tls: kernel TLS support")
>
> INFO: task syz-executor279:9995 blocked for more than 143 seconds.
> Not tainted 5.3.0-rc7+ #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> syz-executor279 D24264 9995 9994 0x00000000
> Call Trace:
> context_switch kernel/sched/core.c:3254 [inline]
> __schedule+0x755/0x1580 kernel/sched/core.c:3880
> schedule+0xd9/0x260 kernel/sched/core.c:3947
> schedule_timeout+0x717/0xc50 kernel/time/timer.c:1783
> do_wait_for_common kernel/sched/completion.c:83 [inline]
> __wait_for_common kernel/sched/completion.c:104 [inline]
> wait_for_common kernel/sched/completion.c:115 [inline]
> wait_for_completion+0x29c/0x440 kernel/sched/completion.c:136
> __flush_work+0x508/0xa50 kernel/workqueue.c:3040
> __cancel_work_timer+0x3d9/0x540 kernel/workqueue.c:3127
> cancel_delayed_work_sync+0x1b/0x20 kernel/workqueue.c:3259
> tls_sw_cancel_work_tx+0x68/0x80 net/tls/tls_sw.c:2063
> tls_sk_proto_close+0x4ac/0x990 net/tls/tls_main.c:299
> inet_release+0xed/0x200 net/ipv4/af_inet.c:427
> inet6_release+0x53/0x80 net/ipv6/af_inet6.c:470
> __sock_release+0xce/0x280 net/socket.c:590
> sock_close+0x1e/0x30 net/socket.c:1268
> __fput+0x2ff/0x890 fs/file_table.c:280
> ____fput+0x16/0x20 fs/file_table.c:313
> task_work_run+0x145/0x1c0 kernel/task_work.c:113
> tracehook_notify_resume include/linux/tracehook.h:188 [inline]
> exit_to_usermode_loop+0x316/0x380 arch/x86/entry/common.c:163
> prepare_exit_to_usermode arch/x86/entry/common.c:194 [inline]
> syscall_return_slowpath arch/x86/entry/common.c:274 [inline]
> do_syscall_64+0x5a9/0x6a0 arch/x86/entry/common.c:299
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x401f40
> Code: ff ff ff 25 62 63 20 00 68 08 00 00 00 e9 60 ff ff ff ff 25 5a 63 20
> 00 68 09 00 00 00 e9 50 ff ff ff ff 25 52 63 20 00 68 0a <00> 00 00 e9 40 ff
> ff ff ff 25 4a 63 20 00 68 0b 00 00 00 e9 30 ff
> RSP: 002b:00007fffd8200d58 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> RAX: 0000000000000000 RBX: 0000000000000005 RCX: 0000000000401f40
> RDX: ffffffffffffffc1 RSI: 1201000000003618 RDI: 0000000000000004
> RBP: 00007fffd8200d70 R08: 0000000000000000 R09: 1201000000003618
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 0000000000403170 R14: 0000000000000000 R15: 0000000000000000
> INFO: lockdep is turned off.
> NMI backtrace for cpu 1
> CPU: 1 PID: 1057 Comm: khungtaskd Not tainted 5.3.0-rc7+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> nmi_cpu_backtrace.cold+0x70/0xb2 lib/nmi_backtrace.c:101
> nmi_trigger_cpumask_backtrace+0x23b/0x28b lib/nmi_backtrace.c:62
> arch_trigger_cpumask_backtrace+0x14/0x20 arch/x86/kernel/apic/hw_nmi.c:38
> trigger_all_cpu_backtrace include/linux/nmi.h:146 [inline]
> check_hung_uninterruptible_tasks kernel/hung_task.c:205 [inline]
> watchdog+0x9d0/0xef0 kernel/hung_task.c:289
> kthread+0x361/0x430 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> Sending NMI from CPU 1 to CPUs 0:
> NMI backtrace for cpu 0 skipped: idling at native_safe_halt+0xe/0x10
> arch/x86/include/asm/irqflags.h:60
>
Reproducer involves pcrypt, so probably the pcrypt deadlock again...
https://lkml.kernel.org/linux-crypto/20190817054743.GE8209@sol.localdomain/
#syz dup: INFO: task hung in aead_recvmsg
^ permalink raw reply
* Re: [Patch net] net_sched: add max len check for TCA_KIND
From: Cong Wang @ 2019-09-19 5:15 UTC (permalink / raw)
To: David Ahern
Cc: Linux Kernel Network Developers, syzbot, Jamal Hadi Salim,
Jiri Pirko
In-Reply-To: <36471b0d-cc83-40aa-3ded-39e864dcceb0@gmail.com>
On Wed, Sep 18, 2019 at 7:41 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 9/18/19 5:24 PM, Cong Wang wrote:
> > The TCA_KIND attribute is of NLA_STRING which does not check
> > the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> > is likely caused by the lack of NUL.
> >
> > Change it to NLA_NUL_STRING and add a max len too.
> >
> > Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>
> The commit referenced here did not introduce the ability to go beyond
> memory boundaries with string comparisons. Rather, it was not complete
> solution for attribute validation. I say that wrt to the fix getting
> propagated to the correct stable releases.
I think this patch should be backported to wherever commit 8b4c3cdd9dd8
goes, this is why I picked it as Fixes.
>
> > Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
>
> What is the actual sysbot report?
https://marc.info/?l=linux-kernel&m=156862916112881&w=2
^ permalink raw reply
* Re: udp sendmsg ENOBUFS clarification
From: Josh Hunt @ 2019-09-19 2:59 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, Eric Dumazet, David Miller
In-Reply-To: <CA+FuTSc3O4XQAmtyY5Fwy96nL17ewdCouvwAJ=6DeMUcQUiz8A@mail.gmail.com>
On 9/18/19 8:35 AM, Willem de Bruijn wrote:
> On Tue, Sep 17, 2019 at 4:20 PM Josh Hunt <johunt@akamai.com> wrote:
>>
>> I was running some tests recently with the udpgso_bench_tx benchmark in
>> selftests and noticed that in some configurations it reported sending
>> more than line rate! Looking into it more I found that I was overflowing
>> the qdisc queue and so it was sending back NET_XMIT_DROP however this
>> error did not propagate back up to the application and so it assumed
>> whatever it sent was done successfully. That's when I learned about
>> IP_RECVERR and saw that the benchmark isn't using that socket option.
>>
>> That's all fairly straightforward, but what I was hoping to get
>> clarification on is where is the line drawn on when or when not to send
>> ENOBUFS back to the application if IP_RECVERR is *not* set? My guess
>> based on going through the code is that as long as the packet leaves the
>> stack (in this case sent to the qdisc) that's where we stop reporting
>> ENOBUFS back to the application, but can someone confirm?
>
> Once a packet is queued the system call may return, so any subsequent
> drops after dequeue are not propagated back. The relevant rc is set in
> __dev_xmit_skb on q->enqueue. On setups with multiple devices, such as
> a tunnel or bonding path, enqueue on the lower device is similar not
> propagated.
Yeah that makes total sense. Once it's enqueued you'd expect it to not
be able to return an error, but in this particular case we get an error
on enqueue so was surprised when it did not get back to the application.
>
>> For example, we sanitize the error in udp_send_skb():
>> send:
>> err = ip_send_skb(sock_net(sk), skb);
>> if (err) {
>> if (err == -ENOBUFS && !inet->recverr) {
>> UDP_INC_STATS(sock_net(sk),
>> UDP_MIB_SNDBUFERRORS, is_udplite);
>> err = 0;
>> }
>> } else
>>
>>
>> but in udp_sendmsg() we don't:
>>
>> if (err == -ENOBUFS || test_bit(SOCK_NOSPACE,
>> &sk->sk_socket->flags)) {
>> UDP_INC_STATS(sock_net(sk),
>> UDP_MIB_SNDBUFERRORS, is_udplite);
>> }
>> return err;
>
> That's interesting. My --incorrect-- understanding until now had been
> that IP_RECVERR does nothing but enable optional extra detailed error
> reporting on top of system call error codes.
>
> But indeed it enables backpressure being reported as a system call
> error that is suppressed otherwise. I don't know why. The behavior
> precedes git history.
Yeah it's interesting. I wasn't able to find any documentation or
discussion on it either which is why I figured I'd ask the question on
netdev in case others know.
>
>> In the case above it looks like we may only get ENOBUFS for allocation
>> failures inside of the stack in udp_sendmsg() and so that's why we
>> propagate the error back up to the application?
>
> Both the udp lockless fast path and the slow corked path go through
> udp_send_skb, so the backpressure is suppressed consistently across
> both cases.
>
> Indeed the error handling in udp_sendmsg then is not related to
> backpressure, but to other causes of ENOBUF, i.e., allocation failure.
>
Yep. Thanks for going through this. We'll see if others have any
comments. I will likely send a patch for the man page adding that you
can get ENOBUFS on Linux but need to set IP_RECVERR as Eric pointed out
in the patch I linked to previously.
Josh
^ permalink raw reply
* [PATCH v4 1/3] kernel/notifier.c: intercepting duplicate registrations to avoid infinite loops
From: Xiaoming Ni @ 2019-09-19 2:58 UTC (permalink / raw)
To: gregkh, akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan,
bfields, chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey,
paulmck, semen.protsenko, stern, tglx, trond.myklebust,
viresh.kumar
Cc: stable, dylix.dailei, nixiaoming, yuehaibing, linux-kernel,
linux-nfs, netdev
In-Reply-To: <1568861888-34045-1-git-send-email-nixiaoming@huawei.com>
Registering the same notifier to a hook repeatedly can cause the hook
list to form a ring or lose other members of the list.
case1: An infinite loop in notifier_chain_register() can cause soft lockup
atomic_notifier_chain_register(&test_notifier_list, &test1);
atomic_notifier_chain_register(&test_notifier_list, &test1);
atomic_notifier_chain_register(&test_notifier_list, &test2);
case2: An infinite loop in notifier_chain_register() can cause soft lockup
atomic_notifier_chain_register(&test_notifier_list, &test1);
atomic_notifier_chain_register(&test_notifier_list, &test1);
atomic_notifier_call_chain(&test_notifier_list, 0, NULL);
case3: lose other hook test2
atomic_notifier_chain_register(&test_notifier_list, &test1);
atomic_notifier_chain_register(&test_notifier_list, &test2);
atomic_notifier_chain_register(&test_notifier_list, &test1);
case4: Unregister returns 0, but the hook is still in the linked list,
and it is not really registered. If you call notifier_call_chain
after ko is unloaded, it will trigger oops.
If the system is configured with softlockup_panic and the same
hook is repeatedly registered on the panic_notifier_list, it
will cause a loop panic.
Add a check in notifier_chain_register(),
Intercepting duplicate registrations to avoid infinite loops
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
Reviewed-by: Vasily Averin <vvs@virtuozzo.com>
---
kernel/notifier.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/notifier.c b/kernel/notifier.c
index d9f5081..30bedb8 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -23,7 +23,10 @@ static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
- WARN_ONCE(((*nl) == n), "double register detected");
+ if (unlikely((*nl) == n)) {
+ WARN(1, "double register detected");
+ return 0;
+ }
if (n->priority > (*nl)->priority)
break;
nl = &((*nl)->next);
--
1.8.5.6
^ permalink raw reply related
* [PATCH v4 0/3] kernel/notifier.c: intercepting duplicate registrations to avoid infinite loops
From: Xiaoming Ni @ 2019-09-19 2:58 UTC (permalink / raw)
To: gregkh, akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan,
bfields, chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey,
paulmck, semen.protsenko, stern, tglx, trond.myklebust,
viresh.kumar
Cc: stable, dylix.dailei, nixiaoming, yuehaibing, linux-kernel,
linux-nfs, netdev
Registering the same notifier to a hook repeatedly can cause the hook
list to form a ring or lose other members of the list.
so, need add a check in in notifier_chain_register(),
intercepting duplicate registrations to avoid infinite loops
v1:
* use notifier_chain_cond_register replace notifier_chain_register
v2:
* Add a check in notifier_chain_register() to avoid duplicate registration
* remove notifier_chain_cond_register() to avoid duplicate code
* remove blocking_notifier_chain_cond_register() to avoid duplicate code
v3:
* Add a cover letter.
v4:
* Add Reviewed-by and adjust the title.
Xiaoming Ni (3):
kernel/notifier.c: intercepting duplicate registrations to avoid
infinite loops
kernel/notifier.c: remove notifier_chain_cond_register()
kernel/notifier.c: remove blocking_notifier_chain_cond_register()
include/linux/notifier.h | 4 ----
kernel/notifier.c | 41 +++--------------------------------------
net/sunrpc/rpc_pipe.c | 2 +-
3 files changed, 4 insertions(+), 43 deletions(-)
--
1.8.5.6
^ permalink raw reply
* [PATCH v4 2/3] kernel/notifier.c: remove notifier_chain_cond_register()
From: Xiaoming Ni @ 2019-09-19 2:58 UTC (permalink / raw)
To: gregkh, akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan,
bfields, chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey,
paulmck, semen.protsenko, stern, tglx, trond.myklebust,
viresh.kumar
Cc: stable, dylix.dailei, nixiaoming, yuehaibing, linux-kernel,
linux-nfs, netdev
In-Reply-To: <1568861888-34045-1-git-send-email-nixiaoming@huawei.com>
The only difference between notifier_chain_cond_register() and
notifier_chain_register() is the lack of warning hints for duplicate
registrations.
Consider using notifier_chain_register() instead of
notifier_chain_cond_register() to avoid duplicate code
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
kernel/notifier.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 30bedb8..e3d221f 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -36,21 +36,6 @@ static int notifier_chain_register(struct notifier_block **nl,
return 0;
}
-static int notifier_chain_cond_register(struct notifier_block **nl,
- struct notifier_block *n)
-{
- while ((*nl) != NULL) {
- if ((*nl) == n)
- return 0;
- if (n->priority > (*nl)->priority)
- break;
- nl = &((*nl)->next);
- }
- n->next = *nl;
- rcu_assign_pointer(*nl, n);
- return 0;
-}
-
static int notifier_chain_unregister(struct notifier_block **nl,
struct notifier_block *n)
{
@@ -252,7 +237,7 @@ int blocking_notifier_chain_cond_register(struct blocking_notifier_head *nh,
int ret;
down_write(&nh->rwsem);
- ret = notifier_chain_cond_register(&nh->head, n);
+ ret = notifier_chain_register(&nh->head, n);
up_write(&nh->rwsem);
return ret;
}
--
1.8.5.6
^ permalink raw reply related
* [PATCH v4 3/3] kernel/notifier.c: remove blocking_notifier_chain_cond_register()
From: Xiaoming Ni @ 2019-09-19 2:58 UTC (permalink / raw)
To: gregkh, akpm, vvs, torvalds, adobriyan, anna.schumaker, arjan,
bfields, chuck.lever, davem, jlayton, luto, mingo, Nadia.Derbey,
paulmck, semen.protsenko, stern, tglx, trond.myklebust,
viresh.kumar
Cc: stable, dylix.dailei, nixiaoming, yuehaibing, linux-kernel,
linux-nfs, netdev
In-Reply-To: <1568861888-34045-1-git-send-email-nixiaoming@huawei.com>
blocking_notifier_chain_cond_register() does not consider
system_booting state, which is the only difference between this
function and blocking_notifier_cain_register(). This can be a bug
and is a piece of duplicate code.
Delete blocking_notifier_chain_cond_register()
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
include/linux/notifier.h | 4 ----
kernel/notifier.c | 23 -----------------------
net/sunrpc/rpc_pipe.c | 2 +-
3 files changed, 1 insertion(+), 28 deletions(-)
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 0096a05..0189476 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -150,10 +150,6 @@ extern int raw_notifier_chain_register(struct raw_notifier_head *nh,
extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
struct notifier_block *nb);
-extern int blocking_notifier_chain_cond_register(
- struct blocking_notifier_head *nh,
- struct notifier_block *nb);
-
extern int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
struct notifier_block *nb);
extern int blocking_notifier_chain_unregister(struct blocking_notifier_head *nh,
diff --git a/kernel/notifier.c b/kernel/notifier.c
index e3d221f..63d7501 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -221,29 +221,6 @@ int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
EXPORT_SYMBOL_GPL(blocking_notifier_chain_register);
/**
- * blocking_notifier_chain_cond_register - Cond add notifier to a blocking notifier chain
- * @nh: Pointer to head of the blocking notifier chain
- * @n: New entry in notifier chain
- *
- * Adds a notifier to a blocking notifier chain, only if not already
- * present in the chain.
- * Must be called in process context.
- *
- * Currently always returns zero.
- */
-int blocking_notifier_chain_cond_register(struct blocking_notifier_head *nh,
- struct notifier_block *n)
-{
- int ret;
-
- down_write(&nh->rwsem);
- ret = notifier_chain_register(&nh->head, n);
- up_write(&nh->rwsem);
- return ret;
-}
-EXPORT_SYMBOL_GPL(blocking_notifier_chain_cond_register);
-
-/**
* blocking_notifier_chain_unregister - Remove notifier from a blocking notifier chain
* @nh: Pointer to head of the blocking notifier chain
* @n: Entry to remove from notifier chain
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index b71a39d..39e14d5 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -51,7 +51,7 @@
int rpc_pipefs_notifier_register(struct notifier_block *nb)
{
- return blocking_notifier_chain_cond_register(&rpc_pipefs_notifier_list, nb);
+ return blocking_notifier_chain_register(&rpc_pipefs_notifier_list, nb);
}
EXPORT_SYMBOL_GPL(rpc_pipefs_notifier_register);
--
1.8.5.6
^ permalink raw reply related
* Re: [PATCH RFC] net/phy: fix Marvell PHYs probe failure when HWMON and THERMAL_OF are enabled
From: Andrew Lunn @ 2019-09-19 2:50 UTC (permalink / raw)
To: Peter Mamonov; +Cc: f.fainelli, hkallweit1, netdev
In-Reply-To: <20190918213837.24585-1-pmamonov@gmail.com>
On Thu, Sep 19, 2019 at 12:38:37AM +0300, Peter Mamonov wrote:
> Hello,
>
> Some time ago I've discovered that probe functions of certain Marvell PHYs
> fail if both HWMON and THERMAL_OF config options are enabled.
Hi Peter
It probably affects more then Marvell PHYs.
> The root
> cause of this problem is a lack of an OF node for a PHY's built-in
> temperature sensor. However I consider adding this OF node to be a bit
> excessive solution. Am I wrong? Below you will find a one line patch which
> fixes the problem.
Your patch look sensible to me.
> I've sent it to the releveant maintainers three weeks
> ago without any feedback yet.
Could you point me at the relevant mailing list archive?
Thanks
Andrew
^ permalink raw reply
* Re: [Patch net] net_sched: add max len check for TCA_KIND
From: David Ahern @ 2019-09-19 2:41 UTC (permalink / raw)
To: Cong Wang, netdev
Cc: syzbot+618aacd49e8c8b8486bd, Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <20190918232412.16718-1-xiyou.wangcong@gmail.com>
On 9/18/19 5:24 PM, Cong Wang wrote:
> The TCA_KIND attribute is of NLA_STRING which does not check
> the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> is likely caused by the lack of NUL.
>
> Change it to NLA_NUL_STRING and add a max len too.
>
> Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
The commit referenced here did not introduce the ability to go beyond
memory boundaries with string comparisons. Rather, it was not complete
solution for attribute validation. I say that wrt to the fix getting
propagated to the correct stable releases.
> Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
What is the actual sysbot report?
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> net/sched/sch_api.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 1047825d9f48..81d58b280612 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1390,7 +1390,8 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
> }
>
> const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> - [TCA_KIND] = { .type = NLA_STRING },
> + [TCA_KIND] = { .type = NLA_NUL_STRING,
> + .len = IFNAMSIZ - 1 },
> [TCA_RATE] = { .type = NLA_BINARY,
> .len = sizeof(struct tc_estimator) },
> [TCA_STAB] = { .type = NLA_NESTED },
>
This is a better policy so for that:
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH RFC v3 2/5] net: Add NETIF_F_GRO_LIST feature
From: Subash Abhinov Kasiviswanathan @ 2019-09-19 2:04 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Steffen Klassert, Network Development, Paolo Abeni
In-Reply-To: <CA+FuTSeBmGY4_2X3Ydhf60G=An9g9iikDBQMDji=XptN_jBqiw@mail.gmail.com>
On 2019-09-18 10:10, Willem de Bruijn wrote:
> On Wed, Sep 18, 2019 at 3:25 AM Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
>>
>> This adds a new NETIF_F_GRO_LIST feature flag. I will be used
>> to configure listfyed GRO what will be implemented with some
>> followup paches.
>
> This should probably simultaneously introduce SKB_GSO_FRAGLIST as well
> as a BUILD_BUG_ON in net_gso_ok.
>
> Please also in the commit describe the constraints of skbs that have
> this type. If I'm not mistaken, an skb with either gso_size linear
> data or one gso_sized frag, followed by a frag_list of the same. With
> the exception of the last frag_list member, whose mss may be less than
> gso_size. This will help when reasoning about all the types of skbs we
> may see at segmentation, as we recently had to do [1]
>
Would it be preferrable to allow any size skbs for the listification.
Since the original skbs are being restored, single gso_size shoudln't
be a constraint here.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [Patch net] net_sched: add policy validation for action attributes
From: Cong Wang @ 2019-09-19 1:44 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David Ahern, Jamal Hadi Salim, Jiri Pirko
Similar to commit 8b4c3cdd9dd8
("net: sched: Add policy validation for tc attributes"), we need
to add proper policy validation for TC action attributes too.
Cc: David Ahern <dsahern@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/act_api.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 835adde28a7e..da99667589f8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -831,6 +831,15 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
return c;
}
+static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
+ [TCA_ACT_KIND] = { .type = NLA_NUL_STRING,
+ .len = IFNAMSIZ - 1 },
+ [TCA_ACT_INDEX] = { .type = NLA_U32 },
+ [TCA_ACT_COOKIE] = { .type = NLA_BINARY,
+ .len = TC_COOKIE_MAX_SIZE },
+ [TCA_ACT_OPTIONS] = { .type = NLA_NESTED },
+};
+
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
@@ -846,8 +855,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
int err;
if (name == NULL) {
- err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla, NULL,
- extack);
+ err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla,
+ tcf_action_policy, extack);
if (err < 0)
goto err_out;
err = -EINVAL;
@@ -856,18 +865,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
NL_SET_ERR_MSG(extack, "TC action kind must be specified");
goto err_out;
}
- if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ) {
- NL_SET_ERR_MSG(extack, "TC action name too long");
- goto err_out;
- }
- if (tb[TCA_ACT_COOKIE]) {
- int cklen = nla_len(tb[TCA_ACT_COOKIE]);
-
- if (cklen > TC_COOKIE_MAX_SIZE) {
- NL_SET_ERR_MSG(extack, "TC cookie size above the maximum");
- goto err_out;
- }
+ nla_strlcpy(act_name, kind, IFNAMSIZ);
+ if (tb[TCA_ACT_COOKIE]) {
cookie = nla_memdup_cookie(tb);
if (!cookie) {
NL_SET_ERR_MSG(extack, "No memory to generate TC cookie");
@@ -1097,7 +1097,8 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
int index;
int err;
- err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla, NULL, extack);
+ err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla,
+ tcf_action_policy, extack);
if (err < 0)
goto err_out;
@@ -1151,7 +1152,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
b = skb_tail_pointer(skb);
- err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla, NULL, extack);
+ err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX, nla,
+ tcf_action_policy, extack);
if (err < 0)
goto err_out;
@@ -1439,7 +1441,7 @@ static struct nlattr *find_dump_kind(struct nlattr **nla)
if (tb[1] == NULL)
return NULL;
- if (nla_parse_nested_deprecated(tb2, TCA_ACT_MAX, tb[1], NULL, NULL) < 0)
+ if (nla_parse_nested_deprecated(tb2, TCA_ACT_MAX, tb[1], tcf_action_policy, NULL) < 0)
return NULL;
kind = tb2[TCA_ACT_KIND];
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2] rtl8xxxu: add bluetooth co-existence support for single antenna
From: Chris Chiu @ 2019-09-19 1:44 UTC (permalink / raw)
To: Jes Sorensen, Kalle Valo, David Miller
Cc: linux-wireless, netdev, Linux Kernel, Linux Upstreaming Team
In-Reply-To: <20190911025045.20918-1-chiu@endlessm.com>
On Wed, Sep 11, 2019 at 10:50 AM Chris Chiu <chiu@endlessm.com> wrote:
>
>
> Notes:
> v2:
> - Add helper functions to replace bunch of tdma settings
> - Reformat some lines to meet kernel coding style
>
>
Gentle ping. Any suggestions would be appreciated. Thanks.
Chris
^ permalink raw reply
* [PATCH ghak90 V7 21/21] audit: add proc interface for capcontid
From: Richard Guy Briggs @ 2019-09-19 1:22 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: Paul Moore, sgrubb, omosnace, dhowells, simo, eparis, serge,
ebiederm, nhorman, dwalsh, mpatel, Richard Guy Briggs
In-Reply-To: <cover.1568834524.git.rgb@redhat.com>
Add a /proc interface to capcontid for testing purposes. This isn't
intended to be merged upstream. Container orchestrators/engines are
expected to link to libaudit to use the functions audit_set_capcontid()
and audit_get_capcontid.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
fs/proc/base.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 26091800180c..283ef8e006e7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1360,6 +1360,59 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
.write = proc_contid_write,
.llseek = generic_file_llseek,
};
+
+static ssize_t proc_capcontid_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct inode *inode = file_inode(file);
+ struct task_struct *task = get_proc_task(inode);
+ ssize_t length;
+ char tmpbuf[TMPBUFLEN];
+
+ if (!task)
+ return -ESRCH;
+ /* if we don't have caps, reject */
+ if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+ return -EPERM;
+ length = scnprintf(tmpbuf, TMPBUFLEN, "%u", audit_get_capcontid(task));
+ put_task_struct(task);
+ return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
+static ssize_t proc_capcontid_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct inode *inode = file_inode(file);
+ u32 capcontid;
+ int rv;
+ struct task_struct *task = get_proc_task(inode);
+
+ if (!task)
+ return -ESRCH;
+ if (*ppos != 0) {
+ /* No partial writes. */
+ put_task_struct(task);
+ return -EINVAL;
+ }
+
+ rv = kstrtou32_from_user(buf, count, 10, &capcontid);
+ if (rv < 0) {
+ put_task_struct(task);
+ return rv;
+ }
+
+ rv = audit_set_capcontid(task, capcontid);
+ put_task_struct(task);
+ if (rv < 0)
+ return rv;
+ return count;
+}
+
+static const struct file_operations proc_capcontid_operations = {
+ .read = proc_capcontid_read,
+ .write = proc_capcontid_write,
+ .llseek = generic_file_llseek,
+};
#endif
#ifdef CONFIG_FAULT_INJECTION
@@ -3121,6 +3174,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid", S_IRUGO, proc_sessionid_operations),
REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+ REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
#endif
#ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
@@ -3522,6 +3576,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid", S_IRUGO, proc_sessionid_operations),
REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+ REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
#endif
#ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
--
1.8.3.1
^ permalink raw reply related
* [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns
From: Richard Guy Briggs @ 2019-09-19 1:22 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: Paul Moore, sgrubb, omosnace, dhowells, simo, eparis, serge,
ebiederm, nhorman, dwalsh, mpatel, Richard Guy Briggs
In-Reply-To: <cover.1568834524.git.rgb@redhat.com>
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a
process in a non-init user namespace the capability to set audit
container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and
AUDIT_SET_CAPCONTID 1028. The message format includes the data
structure:
struct audit_capcontid_status {
pid_t pid;
u32 enable;
};
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/audit.h | 14 +++++++
include/uapi/linux/audit.h | 2 +
kernel/audit.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-
kernel/audit.h | 5 +++
4 files changed, 117 insertions(+), 2 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 1ce27af686ea..dcc53e62e266 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -117,6 +117,7 @@ struct audit_task_info {
kuid_t loginuid;
unsigned int sessionid;
struct audit_cont *cont;
+ u32 capcontid;
#ifdef CONFIG_AUDITSYSCALL
struct audit_context *ctx;
#endif
@@ -224,6 +225,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
return tsk->audit->sessionid;
}
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+ if (!tsk->audit)
+ return 0;
+ return tsk->audit->capcontid;
+}
+
+extern int audit_set_capcontid(struct task_struct *tsk, u32 enable);
extern int audit_set_contid(struct task_struct *tsk, u64 contid);
static inline u64 audit_get_contid(struct task_struct *tsk)
@@ -309,6 +318,11 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
return AUDIT_SID_UNSET;
}
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+ return 0;
+}
+
static inline u64 audit_get_contid(struct task_struct *tsk)
{
return AUDIT_CID_UNSET;
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index eef42c8eea77..011b0a8ee9b2 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -78,6 +78,8 @@
#define AUDIT_GET_LOGINUID 1024 /* Get loginuid of a task */
#define AUDIT_SET_LOGINUID 1025 /* Set loginuid of a task */
#define AUDIT_GET_SESSIONID 1026 /* Set sessionid of a task */
+#define AUDIT_GET_CAPCONTID 1027 /* Get cap_contid of a task */
+#define AUDIT_SET_CAPCONTID 1028 /* Set cap_contid of a task */
#define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel */
#define AUDIT_USER_AVC 1107 /* We filter this differently */
diff --git a/kernel/audit.c b/kernel/audit.c
index a70c9184e5d9..7160da464849 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1192,6 +1192,14 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
case AUDIT_GET_SESSIONID:
return 0;
break;
+ case AUDIT_GET_CAPCONTID:
+ case AUDIT_SET_CAPCONTID:
+ case AUDIT_GET_CONTID:
+ case AUDIT_SET_CONTID:
+ if (!netlink_capable(skb, CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+ return -EPERM;
+ return 0;
+ break;
default: /* do more checks below */
break;
}
@@ -1227,8 +1235,6 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
case AUDIT_TTY_SET:
case AUDIT_TRIM:
case AUDIT_MAKE_EQUIV:
- case AUDIT_GET_CONTID:
- case AUDIT_SET_CONTID:
case AUDIT_SET_LOGINUID:
/* Only support auditd and auditctl in initial pid namespace
* for now. */
@@ -1304,6 +1310,23 @@ static int audit_get_contid_status(struct sk_buff *skb)
return 0;
}
+static int audit_get_capcontid_status(struct sk_buff *skb)
+{
+ struct nlmsghdr *nlh = nlmsg_hdr(skb);
+ u32 seq = nlh->nlmsg_seq;
+ void *data = nlmsg_data(nlh);
+ struct audit_capcontid_status cs;
+
+ cs.pid = ((struct audit_capcontid_status *)data)->pid;
+ if (!cs.pid)
+ cs.pid = task_tgid_nr(current);
+ rcu_read_lock();
+ cs.enable = audit_get_capcontid(find_task_by_vpid(cs.pid));
+ rcu_read_unlock();
+ audit_send_reply(skb, seq, AUDIT_GET_CAPCONTID, 0, 0, &cs, sizeof(cs));
+ return 0;
+}
+
struct audit_loginuid_status { uid_t loginuid; };
static int audit_get_loginuid_status(struct sk_buff *skb)
@@ -1779,6 +1802,27 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err)
return err;
break;
+ case AUDIT_SET_CAPCONTID: {
+ struct audit_capcontid_status *s = data;
+ struct task_struct *tsk;
+
+ /* check if new data is valid */
+ if (nlmsg_len(nlh) < sizeof(*s))
+ return -EINVAL;
+ tsk = find_get_task_by_vpid(s->pid);
+ if (!tsk)
+ return -EINVAL;
+
+ err = audit_set_capcontid(tsk, s->enable);
+ put_task_struct(tsk);
+ return err;
+ break;
+ }
+ case AUDIT_GET_CAPCONTID:
+ err = audit_get_capcontid_status(skb);
+ if (err)
+ return err;
+ break;
case AUDIT_SET_LOGINUID: {
uid_t *loginuid = data;
kuid_t kloginuid;
@@ -2711,6 +2755,56 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
return NULL;
}
+int audit_set_capcontid(struct task_struct *task, u32 enable)
+{
+ u32 oldcapcontid;
+ int rc = 0;
+ struct audit_buffer *ab;
+ uid_t uid;
+ struct tty_struct *tty;
+ char comm[sizeof(current->comm)];
+
+ if (!task->audit)
+ return -ENOPROTOOPT;
+ oldcapcontid = audit_get_capcontid(task);
+ /* if task is not descendant, block */
+ if (task == current)
+ rc = -EBADSLT;
+ else if (!task_is_descendant(current, task))
+ rc = -EXDEV;
+ else if (current_user_ns() == &init_user_ns) {
+ if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+ rc = -EPERM;
+ }
+ if (!rc)
+ task->audit->capcontid = enable;
+
+ if (!audit_enabled)
+ return rc;
+
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_SET_CAPCONTID);
+ if (!ab)
+ return rc;
+
+ uid = from_kuid(&init_user_ns, task_uid(current));
+ tty = audit_get_tty();
+ audit_log_format(ab,
+ "opid=%d capcontid=%u old-capcontid=%u pid=%d uid=%u auid=%u tty=%s ses=%u",
+ task_tgid_nr(task), enable, oldcapcontid,
+ task_tgid_nr(current), uid,
+ from_kuid(&init_user_ns, audit_get_loginuid(current)),
+ tty ? tty_name(tty) : "(none)",
+ audit_get_sessionid(current));
+ audit_put_tty(tty);
+ audit_log_task_context(ab);
+ audit_log_format(ab, " comm=");
+ audit_log_untrustedstring(ab, get_task_comm(comm, current));
+ audit_log_d_path_exe(ab, current->mm);
+ audit_log_format(ab, " res=%d", !rc);
+ audit_log_end(ab);
+ return rc;
+}
+
/*
* audit_set_contid - set current task's audit contid
* @task: target task
diff --git a/kernel/audit.h b/kernel/audit.h
index cb25341c1a0f..ac4694e88485 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -231,6 +231,11 @@ struct audit_contid_status {
u64 id;
};
+struct audit_capcontid_status {
+ pid_t pid;
+ u32 enable;
+};
+
#define AUDIT_CONTID_DEPTH 5
/* Indicates that audit should log the full pathname. */
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox