2017-03-22 15:08:50

by Stanislaw Gruszka

Subject: [PATCH 4.11] genetlink: fix counting regression on ctrl_dumpfamily()

Commit 2ae0f17df1cd ("genetlink: use idr to track families") replaced

if (++n < fams_to_skip)
continue;
into:

if (n++ < fams_to_skip)
continue;

This subtle change cause that on retry ctrl_dumpfamily() call we omit
one family that failed to do ctrl_fill_info() on previous call, because
cb->args[0] = n number counts also family that failed to do
ctrl_fill_info().

Patch fixes the problem and avoid confusion in the future just decrease
n counter when ctrl_fill_info() fail.

User visible problem caused by this bug is failure to get access to
some genetlink family i.e. nl80211. However problem is reproducible
only if number of registered genetlink families is big enough to
cause second call of ctrl_dumpfamily().

Cc: Xose Vazquez Perez <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Johannes Berg <[email protected]>
Fixes: 2ae0f17df1cd ("genetlink: use idr to track families")
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
Dave, please also target this for 4.10+ -stable.

net/netlink/genetlink.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index fb6e10f..92e0981 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -783,8 +783,10 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)

if (ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
- skb, CTRL_CMD_NEWFAMILY) < 0)
+ skb, CTRL_CMD_NEWFAMILY) < 0) {
+ n--;
break;
+ }
}

cb->args[0] = n;
--
1.7.1

2017-03-22 22:39:16

by David Miller

Subject: Re: [PATCH 4.11] genetlink: fix counting regression on ctrl_dumpfamily()

From: Stanislaw Gruszka <[email protected]>
Date: Wed, 22 Mar 2017 16:08:33 +0100

> Commit 2ae0f17df1cd ("genetlink: use idr to track families") replaced
>
> if (++n < fams_to_skip)
> continue;
> into:
>
> if (n++ < fams_to_skip)
> continue;
>
> This subtle change cause that on retry ctrl_dumpfamily() call we omit
> one family that failed to do ctrl_fill_info() on previous call, because
> cb->args[0] = n number counts also family that failed to do
> ctrl_fill_info().
>
> Patch fixes the problem and avoid confusion in the future just decrease
> n counter when ctrl_fill_info() fail.
>
> User visible problem caused by this bug is failure to get access to
> some genetlink family i.e. nl80211. However problem is reproducible
> only if number of registered genetlink families is big enough to
> cause second call of ctrl_dumpfamily().
>
> Cc: Xose Vazquez Perez <[email protected]>
> Cc: Larry Finger <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Fixes: 2ae0f17df1cd ("genetlink: use idr to track families")
> Signed-off-by: Stanislaw Gruszka <[email protected]>

Applied and queued up for -stable, thanks.

2017-03-22 22:05:29

by Johannes Berg

Subject: Re: [PATCH 4.11] genetlink: fix counting regression on ctrl_dumpfamily()

On Wed, 2017-03-22 at 16:08 +0100, Stanislaw Gruszka wrote:
> Commit 2ae0f17df1cd ("genetlink: use idr to track families") replaced
>
> if (++n < fams_to_skip)
> continue;
> into:
>
> if (n++ < fams_to_skip)
> continue;

D'oh. I don't know how that ended up in there, tbh.

Acked-by: Johannes Berg <[email protected]>

johannes

2017-03-23 10:45:21

by poma

Subject: Re: [PATCH 4.11] genetlink: fix counting regression on ctrl_dumpfamily()

On 22.03.2017 16:08, Stanislaw Gruszka wrote:
> Commit 2ae0f17df1cd ("genetlink: use idr to track families") replaced
>
> if (++n < fams_to_skip)
> continue;
> into:
>
> if (n++ < fams_to_skip)
> continue;
>
> This subtle change cause that on retry ctrl_dumpfamily() call we omit
> one family that failed to do ctrl_fill_info() on previous call, because
> cb->args[0] = n number counts also family that failed to do
> ctrl_fill_info().
>
> Patch fixes the problem and avoid confusion in the future just decrease
> n counter when ctrl_fill_info() fail.
>
> User visible problem caused by this bug is failure to get access to
> some genetlink family i.e. nl80211. However problem is reproducible
> only if number of registered genetlink families is big enough to
> cause second call of ctrl_dumpfamily().
>
> Cc: Xose Vazquez Perez <[email protected]>
> Cc: Larry Finger <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Fixes: 2ae0f17df1cd ("genetlink: use idr to track families")
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> Dave, please also target this for 4.10+ -stable.
>
> net/netlink/genetlink.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index fb6e10f..92e0981 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -783,8 +783,10 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
>
> if (ctrl_fill_info(rt, NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
> - skb, CTRL_CMD_NEWFAMILY) < 0)
> + skb, CTRL_CMD_NEWFAMILY) < 0) {
> + n--;
> break;
> + }
> }
>
> cb->args[0] = n;
>


Thanks Stanislaw, Larry!

Tested-by: poma <[email protected]>

Ref.
https://bugzilla.redhat.com/show_bug.cgi?id=1422247