[LU-8915] Don't use linux list structure as user land arguments for lnet selftest Created: 07/Dec/16 Updated: 19/Jan/23 |
|
| Status: | Reopened |
| Project: | Lustre |
| Component/s: | None |
| Affects Version/s: | Lustre 2.10.0 |
| Fix Version/s: | Lustre 2.16.0 |
| Type: | Bug | Priority: | Major |
| Reporter: | James A Simmons | Assignee: | James A Simmons |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||
| Severity: | 3 | ||||||||||||||||
| Rank (Obsolete): | 9223372036854775807 | ||||||||||||||||
| Description |
|
As the upstream client looks like move out of staging issues are being brought up that need to be addressed. One issue is that the lnet selftest utility uses and passes linux kernel link list as arguments. A new proper user land interface has to be developed do we can stop using linux link list arguments as ioctl parameters. |
| Comments |
| Comment by Peter Jones [ 07/Dec/16 ] |
|
Thanks James |
| Comment by James A Simmons [ 07/Dec/16 ] |
|
Before I go changing user land API's Doug, Amir and the rest of the LNet guys need to chime in for what the API should looks like. |
| Comment by Andreas Dilger [ 29/Apr/20 ] |
|
I was looking at this today in the context of patch https://review.whamcloud.com/38105 " What is actually being done is that stuct list_head is being used in userspace to generate a linked list of reply buffers for requests, and the kernel is essentially using the ".next" field in lstcon_rpc_trans_interpreter() as a way of passing a series of userspace pointers to the kernel. The somewhat confusing part is that this is done within a list_for_each_entry() loop in the kernel, but that is actually walking a different list, which is OK. For the "head_up" pointer passed from userspace, each one has the "struct lstcon_rpc_ent" copied properly from userspace, but only the .next field is used from "struct list_head rpe_link" to point to the next "struct lstcon_rpc_ent" structure. The fact that all of these functions pass "struct list_head __user *head_up" as an argument is misleading and makes it look like they could be using list_for_each() in the kernel on this list, but that never happens. Also, since "rpe_link" is the first field in the structure, passing the "head_up" pointer (named result_up in the caller) as the function argument is really the same as passing the pointer to "struct lstcon_rpc_ent" itself. I think it would be possible to change all of these functions and structs in lnet/include/uapi/linux/lnet/lnetst.h from passing "struct list_head __user *" to passing "struct lstcon_rpc_ent __user *" as the argument with no functional difference in the code (even binary compatibility), since all of these pointers end up down in lstcon_rpc_trans_interpreter() at the end. Inside lstcon_rpc_trans_interpreter() instead of "ent = list_entry()" it would use "ent = container_of()" (which list_entry() uses internally these days anyway) to make it clear that it isn't treating this as a list_head. The rpe_link.next field is treated as a 64-bit field that holds a userspace pointer to the next "struct lstcon_rpc_ent", which is only ever accessed via copy_to_user() or compared against itself, which is fine. In summary, I think we can remove the use of "struct list_head" for from the lnetst.h user/kernel interface without even compatibility issues. |
| Comment by Gerrit Updater [ 13/Apr/21 ] |
|
James Simmons (jsimmons@infradead.org) uploaded a new patch: https://review.whamcloud.com/43298 |
| Comment by Gerrit Updater [ 14/Nov/22 ] |
|
"Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/c/fs/lustre-release/+/43298/ |
| Comment by Peter Jones [ 14/Nov/22 ] |
|
Only patch seems to have landed for 2.16. Should reopen if more is to come |
| Comment by James A Simmons [ 14/Nov/22 ] |
|
More patches coming. |
| Comment by Gerrit Updater [ 05/Dec/22 ] |
|
"James Simmons <jsimmons@infradead.org>" uploaded a new patch: https://review.whamcloud.com/c/fs/lustre-release/+/49314 |
| Comment by Gerrit Updater [ 19/Jan/23 ] |
|
"Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/c/fs/lustre-release/+/49314/ |
| Comment by Peter Jones [ 19/Jan/23 ] |
|
Still more to come? |
| Comment by James A Simmons [ 19/Jan/23 ] |
|
yes |