public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: netdev@vger.kernel.org
Cc: Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	UNGLinuxDriver@microchip.com,
	Xiaoliang Yang <xiaoliang.yang_1@nxp.com>,
	Colin Foster <colin.foster@in-advantage.com>
Subject: [PATCH net 1/6] net: mscc: ocelot: don't use list_empty() on non-initialized list element
Date: Tue,  3 May 2022 14:57:23 +0300	[thread overview]
Message-ID: <20220503115728.834457-2-vladimir.oltean@nxp.com> (raw)
In-Reply-To: <20220503115728.834457-1-vladimir.oltean@nxp.com>

Since the blamed commit, VCAP filters can appear on more than one list.
If their action is "trap", they are chained on ocelot->traps via
filter->trap_list.

Consequently, when we free a VCAP filter, we must remove it from all
lists it is a member of, including ocelot->traps.

Normally, conditionally removing an element from a list (depending on
whether it is present or not) involves traversing the list, but we
already have a reference to the element, so that isn't really necessary.
Moreover, the operation "list_del(&filter->trap_list)" operation is
fundamentally the same regardless of whether we've iterated through the
list or just happened to have the element. So I thought it would be ok
to check whether the element has been added to a list by calling
list_empty().

However, this does not do the correct thing. list_empty() checks whether
"head->next == head", but in our case, head->next == head->prev == NULL,
and head != NULL. This makes us proceed to call list_del(), which
modifies the prev pointer of the next element, and the next pointer of
the prev element. But the next and prev elements are NULL, so we
dereference those pointers and die.

It would appear that list_empty() is not the function to use to detect
that condition. But if we had previously called INIT_LIST_HEAD() on
&filter->trap_list, then we could use list_empty() to denote whether we
are members of a list (any list).

Although the more "natural" thing seems to be to iterate through
ocelot->traps and only remove the filter from the list if it was a
member of it, it seems pointless to do that.

So fix the bug by calling INIT_LIST_HEAD() on the non-head element.

Fixes: e42bd4ed09aa ("net: mscc: ocelot: keep traps in a list")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 03b5e59d033e..b8617e940063 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -793,6 +793,11 @@ static struct ocelot_vcap_filter
 		filter->egress_port.mask = GENMASK(key_length - 1, 0);
 	}
 
+	/* Allow the filter to be removed from ocelot->traps
+	 * without traversing the list
+	 */
+	INIT_LIST_HEAD(&filter->trap_list);
+
 	return filter;
 }
 
-- 
2.25.1


  reply	other threads:[~2022-05-03 11:57 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-03 11:57 [PATCH net 0/6] Ocelot VCAP fixes Vladimir Oltean
2022-05-03 11:57 ` Vladimir Oltean [this message]
2022-05-03 11:57 ` [PATCH net 2/6] net: mscc: ocelot: avoid use after free with deleted tc-trap rules Vladimir Oltean
2022-05-04 21:10   ` Vladimir Oltean
2022-05-03 11:57 ` [PATCH net 3/6] net: mscc: ocelot: fix last VCAP IS1/IS2 filter persisting in hardware when deleted Vladimir Oltean
2022-05-03 11:57 ` [PATCH net 4/6] net: mscc: ocelot: fix VCAP IS2 filters matching on both lookups Vladimir Oltean
2022-05-03 11:57 ` [PATCH net 5/6] net: mscc: ocelot: restrict tc-trap actions to VCAP IS2 lookup 0 Vladimir Oltean
2022-05-03 11:57 ` [PATCH net 6/6] net: mscc: ocelot: avoid corrupting hardware counters when moving VCAP filters Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220503115728.834457-2-vladimir.oltean@nxp.com \
    --to=vladimir.oltean@nxp.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=andrew@lunn.ch \
    --cc=claudiu.manoil@nxp.com \
    --cc=colin.foster@in-advantage.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=pabeni@redhat.com \
    --cc=vivien.didelot@gmail.com \
    --cc=xiaoliang.yang_1@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox