| Message ID | 20250208011852.31434-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | may miss to set node dead on destroy | expand |
* Wei Yang <richard.weiyang@gmail.com> [250207 20:26]: The subject of this patch set makes the issue sound much more sever than it is. It currently sounds like a memory leak or a UAF, which isn't the case. The root node may remain usable for the duration of the rcu window if it's a leaf node. The impact is pretty minor - you may see the old data on calls that happen in the same rcu window - which is the case anyways. You should also say maple_tree: in the subject since this is going to linux-mm. Not a really big deal since each patch in the series specifies the maple tree. > Per my understanding, on destroy we should set each node dead. But current > code miss this when the maple tree has only the root node. > > The reason is mt_destroy_walk() leverage mte_destroy_descend() to set > node dead, but this is skipped since the only root node is a leaf. > > Patch 1 fixes this. > > When adding a test case, I found we always get the new value even we leave the > old root node not dead. It turns out we always re-walk the tree in mas_walk(). > It looks like a typo on the status check of mas_walk(). > > Patch 2 fixes this. > > Patch 3 add a test case to assert retrieving new value when overwriting the > whole range to a tree with only root node > > Wei Yang (3): > maple_tree: may miss to set node dead on destroy > maple_tree: restart walk on correct status > maple_tree: assert retrieving new value on a tree with only root node > > lib/maple_tree.c | 4 +++- > tools/testing/radix-tree/maple.c | 24 ++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > > -- > 2.34.1 >
On Mon, Feb 10, 2025 at 09:31:28AM -0500, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [250207 20:26]: > >The subject of this patch set makes the issue sound much more sever than >it is. It currently sounds like a memory leak or a UAF, which isn't the >case. > Not intend to exaggerate the impact. Is this one would be better? maple_tree: make sure each node is dead on destroy >The root node may remain usable for the duration of the rcu window if >it's a leaf node. The impact is pretty minor - you may see the old data >on calls that happen in the same rcu window - which is the case anyways. > >You should also say maple_tree: in the subject since this is >going to linux-mm. Not a really big deal since each patch in the series >specifies the maple tree. > Thanks, will add it in next version. >> Per my understanding, on destroy we should set each node dead. But current >> code miss this when the maple tree has only the root node. >> >> The reason is mt_destroy_walk() leverage mte_destroy_descend() to set >> node dead, but this is skipped since the only root node is a leaf. >> >> Patch 1 fixes this. >> >> When adding a test case, I found we always get the new value even we leave the >> old root node not dead. It turns out we always re-walk the tree in mas_walk(). >> It looks like a typo on the status check of mas_walk(). >> >> Patch 2 fixes this. >> >> Patch 3 add a test case to assert retrieving new value when overwriting the >> whole range to a tree with only root node >> >> Wei Yang (3): >> maple_tree: may miss to set node dead on destroy >> maple_tree: restart walk on correct status >> maple_tree: assert retrieving new value on a tree with only root node >> >> lib/maple_tree.c | 4 +++- >> tools/testing/radix-tree/maple.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> -- >> 2.34.1 >>
* Wei Yang <richard.weiyang@gmail.com> [250211 03:11]: > On Mon, Feb 10, 2025 at 09:31:28AM -0500, Liam R. Howlett wrote: > >* Wei Yang <richard.weiyang@gmail.com> [250207 20:26]: > > > >The subject of this patch set makes the issue sound much more sever than > >it is. It currently sounds like a memory leak or a UAF, which isn't the > >case. > > > > Not intend to exaggerate the impact. > > Is this one would be better? > > maple_tree: make sure each node is dead on destroy Not really, you are fixing two nodes, one isn't even to do with the destry/dead node. You are also not making sure each node is dead, but fixing an issue with the leaf node. maple_tree: Fix the replacement of a root leaf node ? > > >The root node may remain usable for the duration of the rcu window if > >it's a leaf node. The impact is pretty minor - you may see the old data > >on calls that happen in the same rcu window - which is the case anyways. > > > >You should also say maple_tree: in the subject since this is > >going to linux-mm. Not a really big deal since each patch in the series > >specifies the maple tree. > > > > Thanks, will add it in next version. > > >> Per my understanding, on destroy we should set each node dead. But current > >> code miss this when the maple tree has only the root node. > >> > >> The reason is mt_destroy_walk() leverage mte_destroy_descend() to set > >> node dead, but this is skipped since the only root node is a leaf. > >> > >> Patch 1 fixes this. > >> > >> When adding a test case, I found we always get the new value even we leave the > >> old root node not dead. It turns out we always re-walk the tree in mas_walk(). > >> It looks like a typo on the status check of mas_walk(). > >> > >> Patch 2 fixes this. > >> > >> Patch 3 add a test case to assert retrieving new value when overwriting the > >> whole range to a tree with only root node > >> > >> Wei Yang (3): > >> maple_tree: may miss to set node dead on destroy > >> maple_tree: restart walk on correct status > >> maple_tree: assert retrieving new value on a tree with only root node > >> > >> lib/maple_tree.c | 4 +++- > >> tools/testing/radix-tree/maple.c | 24 ++++++++++++++++++++++++ > >> 2 files changed, 27 insertions(+), 1 deletion(-) > >> > >> -- > >> 2.34.1 > >> > > -- > Wei Yang > Help you, Help me
On Tue, Feb 11, 2025 at 10:28:53AM -0500, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [250211 03:11]: >> On Mon, Feb 10, 2025 at 09:31:28AM -0500, Liam R. Howlett wrote: >> >* Wei Yang <richard.weiyang@gmail.com> [250207 20:26]: >> > >> >The subject of this patch set makes the issue sound much more sever than >> >it is. It currently sounds like a memory leak or a UAF, which isn't the >> >case. >> > >> >> Not intend to exaggerate the impact. >> >> Is this one would be better? >> >> maple_tree: make sure each node is dead on destroy > >Not really, you are fixing two nodes, one isn't even to do with the >destry/dead node. You are also not making sure each node is dead, but >fixing an issue with the leaf node. > >maple_tree: Fix the replacement of a root leaf node ? > Thanks, it looks more precise. BTW, after addressing your current comments, could I send a v2?
On Tue, Feb 11, 2025 at 10:28:53AM -0500, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [250211 03:11]: >> On Mon, Feb 10, 2025 at 09:31:28AM -0500, Liam R. Howlett wrote: >> >* Wei Yang <richard.weiyang@gmail.com> [250207 20:26]: >> > >> >The subject of this patch set makes the issue sound much more sever than >> >it is. It currently sounds like a memory leak or a UAF, which isn't the >> >case. >> > >> >> Not intend to exaggerate the impact. >> >> Is this one would be better? >> >> maple_tree: make sure each node is dead on destroy > >Not really, you are fixing two nodes, one isn't even to do with the >destry/dead node. You are also not making sure each node is dead, but >fixing an issue with the leaf node. > >maple_tree: Fix the replacement of a root leaf node ? > One more question, would it be better to use this as the subject of patch 1?
Per my understanding, on destroy we should set each node dead. But current code miss this when the maple tree has only the root node. The reason is mt_destroy_walk() leverage mte_destroy_descend() to set node dead, but this is skipped since the only root node is a leaf. Patch 1 fixes this. When adding a test case, I found we always get the new value even we leave the old root node not dead. It turns out we always re-walk the tree in mas_walk(). It looks like a typo on the status check of mas_walk(). Patch 2 fixes this. Patch 3 add a test case to assert retrieving new value when overwriting the whole range to a tree with only root node Wei Yang (3): maple_tree: may miss to set node dead on destroy maple_tree: restart walk on correct status maple_tree: assert retrieving new value on a tree with only root node lib/maple_tree.c | 4 +++- tools/testing/radix-tree/maple.c | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-)