usb_control_msg_send() and usb_control_msg_recv() are highly problematic

DISCUSSION Oliver Neukum 2 participants 3 messages Sep 23, 2020 USB

Share & tools

Oliver Neukum

author

2020-09-23 10:59 UTC #0 lore raw

Hi,

you probably do not want to hear this. I was out of comission for the last week weeks and I should have looked at this more closely.

You may notice that usb_control_msg() for times immemorial has been using GFP_NOIO internally. This is because control messages are needed in a lot of contexts such as SCSI error handling and runtime PM that require GFP_NOIO. IIRC at that time we found ourselves unable to go through all those call chains, so we pulled the nuclear option and slapped a blanket GFP_NOIO on the function.

Today I got a patch that outright deleted a memory allocation with GFP_NOIO, so I looked into this. We are making the same mistake we couldn't fix in the past.
I am afraid the API has to be changed to include memory flags. And we should do this now while the damage is still within limits. I am preparing patches.

Regards
Oliver

Greg KH

maintainer

2020-09-23 11:16 UTC #1 lore raw

Quoted 12 lines · Oliver Neukum

Hi,
you probably do not want to hear this. I was out of comission for the last week weeks and I should have looked at this more closely.
You may notice that usb_control_msg() for times immemorial has been using GFP_NOIO internally. This is because control messages are needed in a lot of contexts such as SCSI error handling and runtime PM that require GFP_NOIO. IIRC at that time we found ourselves unable to go through all those call chains, so we pulled the nuclear option and slapped a blanket GFP_NOIO on the function.

Quoted 6 lines

Today I got a patch that outright deleted a memory allocation with GFP_NOIO, so I looked into this. We are making the same mistake we couldn't fix in the past.
I am afraid the API has to be changed to include memory flags. And we should do this now while the damage is still within limits. I am preparing patches.

How about we always use GPF_NOIO for the calls? That should be fine and make it easier, right?

thanks,

greg k-h

Oliver Neukum

author

2020-09-23 11:28 UTC #2 lore raw

Am Mittwoch, den 23.09.2020, 13:07 +0200 schrieb Greg KH:

Quoted 20 lines · Oliver Neukum

Quoted 12 lines
Hi,
you probably do not want to hear this. I was out of comission for the last week weeks and I should have looked at this more closely.
You may notice that usb_control_msg() for times immemorial has been using GFP_NOIO internally. This is because control messages are needed in a lot of contexts such as SCSI error handling and runtime PM that require GFP_NOIO. IIRC at that time we found ourselves unable to go through all those call chains, so we pulled the nuclear option and slapped a blanket GFP_NOIO on the function.
Quoted 6 lines
Today I got a patch that outright deleted a memory allocation with GFP_NOIO, so I looked into this. We are making the same mistake we couldn't fix in the past.
I am afraid the API has to be changed to include memory flags. And we should do this now while the damage is still within limits. I am preparing patches.

Quoted 2 lines

How about we always use GPF_NOIO for the calls? That should be fine and make it easier, right?

We are supposed to use GFP_NOIO thousands of times without any reason? Usually we try to be better citizens in terms of VM allocations.

Of course this works, but do you want an API that is flawed by design?

Regards
Oliver