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.
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]
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]>
---
drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 ++++++++++-----------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 1d1d4caad680..198c9321bf51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -1630,38 +1630,36 @@ static struct qed_iwarp_listener *
qed_iwarp_get_listener(struct qed_hwfn *p_hwfn,
struct qed_iwarp_cm_info *cm_info)
{
- struct qed_iwarp_listener *listener = NULL;
+ struct qed_iwarp_listener *listener = NULL, *iter;
static const u32 ip_zero[4] = { 0, 0, 0, 0 };
- bool found = false;
- list_for_each_entry(listener,
+ list_for_each_entry(iter,
&p_hwfn->p_rdma_info->iwarp.listen_list,
list_entry) {
- if (listener->port == cm_info->local_port) {
- if (!memcmp(listener->ip_addr,
+ if (iter->port == cm_info->local_port) {
+ if (!memcmp(iter->ip_addr,
ip_zero, sizeof(ip_zero))) {
- found = true;
+ listener = iter;
break;
}
- if (!memcmp(listener->ip_addr,
+ if (!memcmp(iter->ip_addr,
cm_info->local_ip,
sizeof(cm_info->local_ip)) &&
- (listener->vlan == cm_info->vlan)) {
- found = true;
+ iter->vlan == cm_info->vlan) {
+ listener = iter;
break;
}
}
}
- if (found) {
+ if (listener)
DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener found = %p\n",
listener);
- return listener;
- }
+ else
+ DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- return NULL;
+ return listener;
}
static int
--
2.25.1
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
By avoiding the use of the iterator variable after the loop, we can lower
the scope of it to the list traversal macros in the future.
To either continue iterating from that position or skip the iteration
(if the previous iteration was complete) list_prepare_entry() is used.
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/team/team.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b07dde6f0abf..688c4393f099 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2425,17 +2425,17 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
int flags, team_nl_send_func_t *send_func,
struct list_head *sel_opt_inst_list)
{
+ struct team_option_inst *opt_inst, *tmp = NULL;
struct nlattr *option_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_option_inst *opt_inst;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- opt_inst = list_first_entry(sel_opt_inst_list,
- struct team_option_inst, tmp_list);
+ tmp = list_first_entry(sel_opt_inst_list,
+ struct team_option_inst, tmp_list);
start_again:
err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2456,7 +2456,9 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
goto nla_put_failure;
i = 0;
+ opt_inst = list_prepare_entry(tmp, sel_opt_inst_list, tmp_list);
incomplete = false;
+ tmp = NULL;
list_for_each_entry_from(opt_inst, sel_opt_inst_list, tmp_list) {
err = team_nl_fill_one_option_get(skb, team, opt_inst);
if (err) {
@@ -2464,6 +2466,7 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = opt_inst;
break;
}
goto errout;
@@ -2707,14 +2710,14 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
struct nlattr *port_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_port *port;
+ struct team_port *port, *tmp = NULL;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- port = list_first_entry_or_null(&team->port_list,
- struct team_port, list);
+ tmp = list_first_entry_or_null(&team->port_list,
+ struct team_port, list);
start_again:
err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2744,7 +2747,9 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
err = team_nl_fill_one_port_get(skb, one_port);
if (err)
goto errout;
- } else if (port) {
+ } else {
+ port = list_prepare_entry(tmp, &team->port_list, list);
+ tmp = NULL;
list_for_each_entry_from(port, &team->port_list, list) {
err = team_nl_fill_one_port_get(skb, port);
if (err) {
@@ -2752,6 +2757,7 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = port;
break;
}
goto errout;
--
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/dsa/dsa.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89c6c86e746f..645522c4dd4a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,22 +112,21 @@ const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf)
const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol)
{
- struct dsa_tag_driver *dsa_tag_driver;
+ struct dsa_tag_driver *dsa_tag_driver = NULL, *iter;
const struct dsa_device_ops *ops;
- bool found = false;
request_module("%s%d", DSA_TAG_DRIVER_ALIAS, tag_protocol);
mutex_lock(&dsa_tag_drivers_lock);
- list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) {
- ops = dsa_tag_driver->ops;
+ list_for_each_entry(iter, &dsa_tag_drivers_list, list) {
+ ops = iter->ops;
if (ops->proto == tag_protocol) {
- found = true;
+ dsa_tag_driver = iter;
break;
}
}
- if (found) {
+ if (dsa_tag_driver) {
if (!try_module_get(dsa_tag_driver->owner))
ops = ERR_PTR(-ENOPROTOOPT);
} else {
--
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.
Since "found" and "p_ent" need to be equal, "found" should be used
consistently to limit the scope of "p_ent" to the list traversal in
the future.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index d01b9245f811..cbaa2abed660 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -934,10 +934,10 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
u8 fw_return_code,
union event_ring_data *p_data)
{
+ struct qed_spq_entry *found = NULL;
struct qed_spq *p_spq;
- struct qed_spq_entry *p_ent = NULL;
+ struct qed_spq_entry *p_ent;
struct qed_spq_entry *tmp;
- struct qed_spq_entry *found = NULL;
if (!p_hwfn)
return -EINVAL;
@@ -980,7 +980,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
"Complete EQE [echo %04x]: func %p cookie %p)\n",
le16_to_cpu(echo),
- p_ent->comp_cb.function, p_ent->comp_cb.cookie);
+ found->comp_cb.function, found->comp_cb.cookie);
if (found->comp_cb.function)
found->comp_cb.function(p_hwfn, found->comp_cb.cookie, p_data,
fw_return_code);
--
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 [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/qed/qed_dev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 672480c9d195..e920e7dcf66a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -174,7 +174,7 @@ int qed_db_recovery_add(struct qed_dev *cdev,
int qed_db_recovery_del(struct qed_dev *cdev,
void __iomem *db_addr, void *db_data)
{
- struct qed_db_recovery_entry *db_entry = NULL;
+ struct qed_db_recovery_entry *db_entry = NULL, *iter;
struct qed_hwfn *p_hwfn;
int rc = -EINVAL;
@@ -190,12 +190,13 @@ int qed_db_recovery_del(struct qed_dev *cdev,
/* Protect the list */
spin_lock_bh(&p_hwfn->db_recovery_info.lock);
- list_for_each_entry(db_entry,
+ list_for_each_entry(iter,
&p_hwfn->db_recovery_info.list, list_entry) {
/* search according to db_data addr since db_addr is not unique (roce) */
- if (db_entry->db_data == db_data) {
- qed_db_recovery_dp_entry(p_hwfn, db_entry, "Deleting");
- list_del(&db_entry->list_entry);
+ if (iter->db_data == db_data) {
+ qed_db_recovery_dp_entry(p_hwfn, iter, "Deleting");
+ list_del(&iter->list_entry);
+ db_entry = iter;
rc = 0;
break;
}
--
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
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]>
---
.../microchip/sparx5/sparx5_mactable.c | 25 +++++++++----------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index a5837dbe0c7e..bb8d9ce79ac2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -362,8 +362,7 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
unsigned char mac[ETH_ALEN],
u16 vid, u32 cfg2)
{
- struct sparx5_mact_entry *mact_entry;
- bool found = false;
+ struct sparx5_mact_entry *mact_entry = NULL, *iter;
u16 port;
if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_TYPE_GET(cfg2) !=
@@ -378,28 +377,28 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
return;
mutex_lock(&sparx5->mact_lock);
- list_for_each_entry(mact_entry, &sparx5->mact_entries, list) {
- if (mact_entry->vid == vid &&
- ether_addr_equal(mac, mact_entry->mac)) {
- found = true;
- mact_entry->flags |= MAC_ENT_ALIVE;
- if (mact_entry->port != port) {
+ list_for_each_entry(iter, &sparx5->mact_entries, list) {
+ if (iter->vid == vid &&
+ ether_addr_equal(mac, iter->mac)) {
+ iter->flags |= MAC_ENT_ALIVE;
+ if (iter->port != port) {
dev_warn(sparx5->dev, "Entry move: %d -> %d\n",
- mact_entry->port, port);
- mact_entry->port = port;
- mact_entry->flags |= MAC_ENT_MOVED;
+ iter->port, port);
+ iter->port = port;
+ iter->flags |= MAC_ENT_MOVED;
}
/* Entry handled */
+ mact_entry = iter;
break;
}
}
mutex_unlock(&sparx5->mact_lock);
- if (found && !(mact_entry->flags & MAC_ENT_MOVED))
+ if (mact_entry && !(mact_entry->flags & MAC_ENT_MOVED))
/* Present, not moved */
return;
- if (!found) {
+ if (!mact_entry) {
/* Entry not found - now add */
mact_entry = alloc_mact_entry(sparx5, mac, vid, port);
if (!mact_entry)
--
2.25.1