Commit 29d26146 authored by David S. Miller's avatar David S. Miller
Browse files

Merge branch 'sch_sfq-derived-limit' 

Octavian Purdila says:

====================
net_sched: sch_sfq: reject a derived limit of 1

Because sfq parameters can influence each other there can be
situations where although the user sets a limit of 2 it can be lowered
to 1:

$ tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 1 depth 1
$ tc qdisc show dev dummy0
qdisc sfq 1: dev dummy0 root refcnt 2 limit 1p quantum 1514b depth 1 divisor 1024

$ tc qdisc add dev dummy0 handle 1: root sfq limit 2 flows 10 depth 1 divisor 1
$ tc qdisc show dev dummy0
qdisc sfq 2: root refcnt 2 limit 1p quantum 1514b depth 1 divisor 1

As a limit of 1 is invalid, this patch series moves the limit
validation to after all configuration changes have been done. To do
so, the configuration is done in a temporary work area then applied to
the internal state.

The patch series also adds new test cases.

v3:
 - remove a couple of unnecessary comments
 - rearrange local variables to use reverse Christmas tree style
   declaration order

v2: https://lore.kernel.org/all/20250402162750.1671155-1-tavip@google.com/
 - remove tmp struct and directly use local variables

v1: https://lore.kernel.org/all/20250328201634.3876474-1-tavip@google.com/


===================

Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 7f1ff1b3 26e70518

net/sched/sch_sfq.c

+50 −16
Original line number Diff line number Diff line
@@ -631,6 +631,15 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt, 
	struct red_parms *p = NULL;

	struct sk_buff *to_free = NULL;

	struct sk_buff *tail = NULL;

	unsigned int maxflows;

	unsigned int quantum;

	unsigned int divisor;

	int perturb_period;

	u8 headdrop;

	u8 maxdepth;

	int limit;

	u8 flags;





	if (opt->nla_len < nla_attr_size(sizeof(*ctl)))

		return -EINVAL;
@@ -652,39 +661,64 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt, 
		if (!p)

			return -ENOMEM;

	}

	if (ctl->limit == 1) {

		NL_SET_ERR_MSG_MOD(extack, "invalid limit");

		return -EINVAL;

	}



	sch_tree_lock(sch);



	limit = q->limit;

	divisor = q->divisor;

	headdrop = q->headdrop;

	maxdepth = q->maxdepth;

	maxflows = q->maxflows;

	perturb_period = q->perturb_period;

	quantum = q->quantum;

	flags = q->flags;



	/* update and validate configuration */

	if (ctl->quantum)

		q->quantum = ctl->quantum;

	WRITE_ONCE(q->perturb_period, ctl->perturb_period * HZ);

		quantum = ctl->quantum;

	perturb_period = ctl->perturb_period * HZ;

	if (ctl->flows)

		q->maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);

		maxflows = min_t(u32, ctl->flows, SFQ_MAX_FLOWS);

	if (ctl->divisor) {

		q->divisor = ctl->divisor;

		q->maxflows = min_t(u32, q->maxflows, q->divisor);

		divisor = ctl->divisor;

		maxflows = min_t(u32, maxflows, divisor);

	}

	if (ctl_v1) {

		if (ctl_v1->depth)

			q->maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);

			maxdepth = min_t(u32, ctl_v1->depth, SFQ_MAX_DEPTH);

		if (p) {

			swap(q->red_parms, p);

			red_set_parms(q->red_parms,

			red_set_parms(p,

				      ctl_v1->qth_min, ctl_v1->qth_max,

				      ctl_v1->Wlog,

				      ctl_v1->Plog, ctl_v1->Scell_log,

				      NULL,

				      ctl_v1->max_P);

		}

		q->flags = ctl_v1->flags;

		q->headdrop = ctl_v1->headdrop;

		flags = ctl_v1->flags;

		headdrop = ctl_v1->headdrop;

	}

	if (ctl->limit) {

		q->limit = min_t(u32, ctl->limit, q->maxdepth * q->maxflows);

		q->maxflows = min_t(u32, q->maxflows, q->limit);

		limit = min_t(u32, ctl->limit, maxdepth * maxflows);

		maxflows = min_t(u32, maxflows, limit);

	}

	if (limit == 1) {

		sch_tree_unlock(sch);

		kfree(p);

		NL_SET_ERR_MSG_MOD(extack, "invalid limit");

		return -EINVAL;

	}



	/* commit configuration */

	q->limit = limit;

	q->divisor = divisor;

	q->headdrop = headdrop;

	q->maxdepth = maxdepth;

	q->maxflows = maxflows;

	WRITE_ONCE(q->perturb_period, perturb_period);

	q->quantum = quantum;

	q->flags = flags;

	if (p)

		swap(q->red_parms, p);



	qlen = sch->q.qlen;

	while (sch->q.qlen > q->limit) {

tools/testing/selftests/tc-testing/tc-tests/qdiscs/sfq.json

+36 −0
Original line number Diff line number Diff line
@@ -228,5 +228,41 @@ 
        "matchCount": "0",

        "teardown": [

        ]

    },

    {

        "id": "7f8f",

        "name": "Check that a derived limit of 1 is rejected (limit 2 depth 1 flows 1)",

        "category": [

            "qdisc",

            "sfq"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [],

        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root sfq limit 2 depth 1 flows 1",

        "expExitCode": "2",

        "verifyCmd": "$TC qdisc show dev $DUMMY",

        "matchPattern": "sfq",

        "matchCount": "0",

        "teardown": []

    },

    {

        "id": "5168",

        "name": "Check that a derived limit of 1 is rejected (limit 2 depth 1 divisor 1)",

        "category": [

            "qdisc",

            "sfq"

        ],

        "plugins": {

            "requires": "nsPlugin"

        },

        "setup": [],

        "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root sfq limit 2 depth 1 divisor 1",

        "expExitCode": "2",

        "verifyCmd": "$TC qdisc show dev $DUMMY",

        "matchPattern": "sfq",

        "matchCount": "0",

        "teardown": []

    }

]