On Thu, 2020-03-26 at 13:08 -0700, Andrew Morton wrote:
After our server is upgraded to a newer kernel, we found that it continuesly print a warning in the kernel message. The warning is, [832984.946322] netlink: 'irmas.lc': attribute type 1 has an invalid length.
irmas.lc is one of our container monitor daemons, and it will use CGROUPSTATS_CMD_GET to get the cgroupstats, that is similar with tools/accounting/getdelays.c. We can also produce this warning with getdelays. For example, after running bellow command $ ./getdelays -C /sys/fs/cgroup/memory then you can find a warning in dmesg, [61607.229318] netlink: 'getdelays': attribute type 1 has an invalid length.
This warning is introduced in commit 6e237d099fac ("netlink: Relax attr validation for fixed length types"), which is used to check whether attributes using types NLA_U* and NLA_S* have an exact length.
Regarding this issue, the root cause is cgroupstats_cmd_get_policy defines a wrong type as NLA_U32, while it should be NLA_NESTED an its minimal length is NLA_HDRLEN. That is similar to taskstats_cmd_get_policy.
As this behavior change really breaks our application, we'd better cc stable as well.
Can you explain how it breaks the application? I mean, it's really only printing a message to the kernel log in this case? At least that's what you're describing.
I think you may be describing it wrong, because an NLA_NESTED is allowed to be *empty* (but otherwise must have at least 4 bytes just like an NLA_U32).
That said, I'm not even sure I agree that this fix is right? See below.
Is it correct to say that although the code has always been incorrect, but only kernels after 6e237d099fac need this change? If so, I'll add Fixes:6e237d099fac to guide the -stable backporting.
That doesn't really seem right - 6e237d099fac *relaxed* the checks. If anything then it ought to point to 28033ae4e0f5 which may have actually returned an error; but again, need to understand better what really the issue is.
diff --git a/kernel/taskstats.c b/kernel/taskstats.c index e2ac0e3..b90a520 100644 --- a/kernel/taskstats.c +++ b/kernel/taskstats.c @@ -35,8 +35,8 @@ static struct genl_family family; static const struct nla_policy taskstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
- [TASKSTATS_CMD_ATTR_PID] = { .type = NLA_U32 },
- [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
- [TASKSTATS_CMD_ATTR_PID] = { .type = NLA_NESTED },
- [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_NESTED },
I'm not sure where this is coming from - the kernel evidently uses them as nested attributes in *outgoing* data (see mk_reply()), but as NLA_U32 in *incoming* data, (see cmd_attr_pid() and cmd_attr_tgid()).
I would generally recommend not doing such a thing as it's messy, but we do have quite a few such instances cases. In all those cases must the policy list the incoming policy since that's what the kernel uses to validate the attributes.
IOW, this part of the change seems _wrong_.
- Make sure they are always aligned.
*/ static const struct nla_policy cgroupstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
- [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_U32 },
- [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_NESTED },
};
And same here, actually.
johannes
On Fri, Mar 27, 2020 at 4:18 AM Johannes Berg johannes@sipsolutions.net wrote:
On Thu, 2020-03-26 at 13:08 -0700, Andrew Morton wrote:
After our server is upgraded to a newer kernel, we found that it continuesly print a warning in the kernel message. The warning is, [832984.946322] netlink: 'irmas.lc': attribute type 1 has an invalid length.
irmas.lc is one of our container monitor daemons, and it will use CGROUPSTATS_CMD_GET to get the cgroupstats, that is similar with tools/accounting/getdelays.c. We can also produce this warning with getdelays. For example, after running bellow command $ ./getdelays -C /sys/fs/cgroup/memory then you can find a warning in dmesg, [61607.229318] netlink: 'getdelays': attribute type 1 has an invalid length.
This warning is introduced in commit 6e237d099fac ("netlink: Relax attr validation for fixed length types"), which is used to check whether attributes using types NLA_U* and NLA_S* have an exact length.
Regarding this issue, the root cause is cgroupstats_cmd_get_policy defines a wrong type as NLA_U32, while it should be NLA_NESTED an its minimal length is NLA_HDRLEN. That is similar to taskstats_cmd_get_policy.
As this behavior change really breaks our application, we'd better cc stable as well.
Can you explain how it breaks the application? I mean, it's really only printing a message to the kernel log in this case? At least that's what you're describing.
I think you may be describing it wrong, because an NLA_NESTED is allowed to be *empty* (but otherwise must have at least 4 bytes just like an NLA_U32).
That said, I'm not even sure I agree that this fix is right? See below.
Is it correct to say that although the code has always been incorrect, but only kernels after 6e237d099fac need this change? If so, I'll add Fixes:6e237d099fac to guide the -stable backporting.
That doesn't really seem right - 6e237d099fac *relaxed* the checks. If anything then it ought to point to 28033ae4e0f5 which may have actually returned an error; but again, need to understand better what really the issue is.
diff --git a/kernel/taskstats.c b/kernel/taskstats.c index e2ac0e3..b90a520 100644 --- a/kernel/taskstats.c +++ b/kernel/taskstats.c @@ -35,8 +35,8 @@ static struct genl_family family;
static const struct nla_policy taskstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
- [TASKSTATS_CMD_ATTR_PID] = { .type = NLA_U32 },
- [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
- [TASKSTATS_CMD_ATTR_PID] = { .type = NLA_NESTED },
- [TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_NESTED },
I'm not sure where this is coming from - the kernel evidently uses them as nested attributes in *outgoing* data (see mk_reply()), but as NLA_U32 in *incoming* data, (see cmd_attr_pid() and cmd_attr_tgid()).
Thanks for the explanation. The nested attributes is only used in *outgoing* data, rather than the 'incoming' data.
I would generally recommend not doing such a thing as it's messy, but we do have quite a few such instances cases. In all those cases must the policy list the incoming policy since that's what the kernel uses to validate the attributes.
IOW, this part of the change seems _wrong_.
- Make sure they are always aligned.
*/ static const struct nla_policy cgroupstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
- [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_U32 },
- [CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_NESTED },
};
And same here, actually.
johannes
Thanks Yafang
linux-stable-mirror@lists.linaro.org
-
Johannes Berg -
Yafang Shao