Data race in net/mptcp/pm_netlink.c

Data race in net/mptcp/pm_netlink.c

[clingfei]

linux-next weekly scan reports that there might be data race in
net/mptcp/pm_netlink.c, when mptcp_pm_nl_flush_addrs_doit
bitmap_zeroing pernet->id_bit_map at line 1748, there might be a
concurrent read at line 1864.
Should we add a lock to protect pernet->id_bit_map?

The report is listed below.
** CID 1601938:  Concurrent data access violations  (MISSING_LOCK)
/net/mptcp/pm_netlink.c: 1864 in mptcp_pm_nl_dump_addr()


________________________________________________________________________________________________________
*** CID 1601938:  Concurrent data access violations  (MISSING_LOCK)
/net/mptcp/pm_netlink.c: 1864 in mptcp_pm_nl_dump_addr()
1858     int i;
1859
1860     pernet = pm_nl_get_pernet(net);
1861
1862     rcu_read_lock();
1863     for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
>>>     CID 1601938:  Concurrent data access violations  (MISSING_LOCK)
>>>     Accessing "pernet->id_bitmap" without holding lock "pm_nl_pernet.lock". Elsewhere, "pm_nl_pernet.id_bitmap" is written to with "pm_nl_pernet.lock" held 1 out of 1 times.
1864     if (test_bit(i, pernet->id_bitmap)) {
1865     entry = __lookup_addr_by_id(pernet, i);
1866     if (!entry)
1867     break;
1868
1869     if (entry->addr.id <= id)

Re: Data race in net/mptcp/pm_netlink.c

[Matthieu Baerts]

Hello,

On 22/11/2024 08:34, clingfei wrote:
> linux-next weekly scan reports that there might be data race in
> net/mptcp/pm_netlink.c, when mptcp_pm_nl_flush_addrs_doit
> bitmap_zeroing pernet->id_bit_map at line 1748, there might be a
> concurrent read at line 1864.
> Should we add a lock to protect pernet->id_bit_map?

I don't think there is an issue here: if the flush is being done in
parallel to an in progress dump (max 255 addresses), I don't know what
the userspace can really expect. What's important is not to access freed
data, which should be prevented by the 'rcu_read_lock()'.

So it looks like no lock or id_bitmap duplication is needed here. Even
if there was a need for a certain consistency there, these 2 operations
should not happen in parallel, because the MPTCP generic NL family
doesn't have ".parallel_ops = true".

So I think this report is a false positive, no?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.