When the list iterator loop does not exit early the list iterator variable
contains a type-confused pointer to a 'bogus' list element computed based
on the head [1].
Often a 'found' variable is used to ensure the list iterator
variable is only accessed after the loop body if the loop did exit early
(using a break or goto).
In other cases that list iterator variable is used in
combination to access the list member which reverses the invocation of
container_of() and brings back a "safe" pointer to the head of the list.
Since, due to this code patten, there were quite a few bugs discovered [2],
Linus concluded that the rule should be to never use the list iterator
after the loop and introduce a dedicated pointer for that [3].
With the new gnu11 standard, it will now be possible to limit the scope
of the list iterator variable to the traversal loop itself by defining
the variable within the for loop.
This, however, requires to remove all uses of the list iterator after
the loop.
Based on input from Paolo Abeni [4], Vinicius Costa Gomes [5], and
Jakub Kicinski [6], I've splitted all the net-next related changes into
two patch sets, where this is part 1.
v2->v3:
- fix commit authors and signed-off order regarding Vladimir's patches
(Sorry about that, wasn't intentional.)
v1->v2:
- Fixed commit message for PATCH 14/18 and used dedicated variable
pointing to the position (Edward Cree)
- Removed redundant check in mv88e6xxx_port_vlan() (Vladimir Oltean)
- Refactor mv88e6xxx_port_vlan() using separate list iterator functions
(Vladimir Oltean)
- Refactor sja1105_insert_gate_entry() to use separate list iterator
functions (Vladimir Oltean)
- Allow early return in sja1105_insert_gate_entry() if
sja1105_first_entry_longer_than() didn't find any element
(Vladimir Oltean)
- Use list_add_tail() instead of list_add() in sja1105_insert_gate_entry()
(Jakub Kicinski)
- net: netcp: also use separate 'pos' variable instead of duplicating list_add()
Link: https://lwn.net/Articles/887097/ [1]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [2]
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [3]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [4]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [5]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [6]
From: Vladimir Oltean <[email protected]>
sja1105_first_entry_longer_than() does not make use of the full struct
sja1105_gate_entry *e, just of e->interval which is set from the passed
entry_time.
This means that if there is a gate conflict, we have allocated e for
nothing, just to free it later. Reorder the memory allocation and the
function call, to avoid that and simplify the error unwind path.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_vl.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 369be2ac3587..e5ea8eb9ec4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
{
struct sja1105_gate_entry *e;
struct list_head *pos;
- int rc;
+
+ pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
+ entry_time, extack);
+ if (IS_ERR(pos))
+ return PTR_ERR(pos);
e = kzalloc(sizeof(*e), GFP_KERNEL);
if (!e)
@@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
e->rule = rule;
e->gate_state = gate_state;
e->interval = entry_time;
-
- pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
- e->interval, extack);
- if (IS_ERR(pos)) {
- rc = PTR_ERR(pos);
- goto err;
- }
-
list_add(&e->list, pos->prev);
gating_cfg->num_entries++;
return 0;
-err:
- kfree(e);
- return rc;
}
/* The gate entries contain absolute times in their e->interval field. Convert
--
2.25.1
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..3d167e37e654 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
{
struct qede_dev *edev = netdev_priv(dev);
- struct qede_vlan *vlan;
+ struct qede_vlan *vlan = NULL;
+ struct qede_vlan *iter;
int rc = 0;
DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);
/* Find whether entry exists */
__qede_lock(edev);
- list_for_each_entry(vlan, &edev->vlan_list, list)
- if (vlan->vid == vid)
+ list_for_each_entry(iter, &edev->vlan_list, list)
+ if (iter->vid == vid) {
+ vlan = iter;
break;
+ }
- if (list_entry_is_head(vlan, &edev->vlan_list, list)) {
+ if (!vlan) {
DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
"Vlan isn't configured\n");
goto out;
--
2.25.1
From: Vladimir Oltean <[email protected]>
We know that "dev > dst->last_switch" in the "else" block.
In other words, that "dev - dst->last_switch" is > 0.
dsa_port_bridge_num_get(dp) can be 0, but the check
"if (bridge_num + dst->last_switch != dev) continue", rewritten as
"if (bridge_num != dev - dst->last_switch) continue", aka
"if (bridge_num != something which cannot be 0) continue",
makes it redundant to have the extra "if (!bridge_num) continue" logic,
since a bridge_num of zero would have been skipped anyway.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..b3aa0e5bc842 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
list_for_each_entry(dp, &dst->ports, list) {
unsigned int bridge_num = dsa_port_bridge_num_get(dp);
- if (!bridge_num)
- continue;
-
if (bridge_num + dst->last_switch != dev)
continue;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
.../net/ethernet/toshiba/ps3_gelic_wireless.c | 30 +++++++++----------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index dc14a66583ff..c8a016c902cd 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1495,14 +1495,14 @@ static int gelic_wl_start_scan(struct gelic_wl_info *wl, int always_scan,
*/
static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
{
+ struct gelic_wl_scan_info *target = NULL, *iter, *tmp;
struct gelic_eurus_cmd *cmd = NULL;
- struct gelic_wl_scan_info *target, *tmp;
struct gelic_wl_scan_info *oldest = NULL;
struct gelic_eurus_scan_info *scan_info;
unsigned int scan_info_size;
union iwreq_data data;
unsigned long this_time = jiffies;
- unsigned int data_len, i, found, r;
+ unsigned int data_len, i, r;
void *buf;
pr_debug("%s:start\n", __func__);
@@ -1539,14 +1539,14 @@ static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
wl->scan_stat = GELIC_WL_SCAN_STAT_GOT_LIST;
/* mark all entries are old */
- list_for_each_entry_safe(target, tmp, &wl->network_list, list) {
- target->valid = 0;
+ list_for_each_entry_safe(iter, tmp, &wl->network_list, list) {
+ iter->valid = 0;
/* expire too old entries */
- if (time_before(target->last_scanned + wl->scan_age,
+ if (time_before(iter->last_scanned + wl->scan_age,
this_time)) {
- kfree(target->hwinfo);
- target->hwinfo = NULL;
- list_move_tail(&target->list, &wl->network_free_list);
+ kfree(iter->hwinfo);
+ iter->hwinfo = NULL;
+ list_move_tail(&iter->list, &wl->network_free_list);
}
}
@@ -1569,22 +1569,22 @@ static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
continue;
}
- found = 0;
+ target = NULL;
oldest = NULL;
- list_for_each_entry(target, &wl->network_list, list) {
- if (ether_addr_equal(&target->hwinfo->bssid[2],
+ list_for_each_entry(iter, &wl->network_list, list) {
+ if (ether_addr_equal(&iter->hwinfo->bssid[2],
&scan_info->bssid[2])) {
- found = 1;
+ target = iter;
pr_debug("%s: same BBS found scanned list\n",
__func__);
break;
}
if (!oldest ||
- (target->last_scanned < oldest->last_scanned))
- oldest = target;
+ (iter->last_scanned < oldest->last_scanned))
+ oldest = iter;
}
- if (!found) {
+ if (!target) {
/* not found in the list */
if (list_empty(&wl->network_free_list)) {
/* expire oldest */
--
2.25.1