Messages in this thread | | | Subject | Re: [PATCH] netxen: Fix a sleep-in-atomic bug in netxen_nic_pci_mem_access_direct | From | Jia-Ju Bai <> | Date | Wed, 21 Jun 2017 22:32:26 +0800 |
| |
On 2017/6/21 21:40, Kalle Valo wrote:
> Jia-Ju Bai <baijiaju1990@163.com> writes: > >> On 06/21/2017 02:11 PM, Kalle Valo wrote: >>> David Miller<davem@davemloft.net> writes: >>> >>>> From: Jia-Ju Bai<baijiaju1990@163.com> >>>> Date: Mon, 19 Jun 2017 10:48:53 +0800 >>>> >>>>> The driver may sleep under a spin lock, and the function call path is: >>>>> netxen_nic_pci_mem_access_direct (acquire the lock by spin_lock) >>>>> ioremap --> may sleep >>>>> >>>>> To fix it, the lock is released before "ioremap", and the lock is >>>>> acquired again after this function. >>>>> >>>>> Signed-off-by: Jia-Ju Bai<baijiaju1990@163.com> >>>> This style of change you are making is really starting to be a >>>> problem. >>>> >>>> You can't just drop locks like this, especially without explaining >>>> why it's ok, and why the mutual exclusion this code was trying to >>>> achieve is still going to be OK afterwards. >>>> >>>> In fact, I see zero analysis of the locking situation here, why >>>> it was needed in the first place, and why your change is OK in >>>> that context. >>>> >>>> Any locking change is delicate, and you must put the greatest of >>>> care and consideration into it. >>>> >>>> Just putting "unlock/lock" around the sleeping operation shows a >>>> very low level of consideration for the implications of the change >>>> you are making. >>>> >>>> This isn't like making whitespace fixes, sorry... >>> We already tried to explain this to Jia-Ju during review of a wireless >>> patch: >>> >>> https://patchwork.kernel.org/patch/9756585/ >>> >>> Jia-Ju, you should listen to feedback. If you continue submitting random >>> patches like this makes it hard for maintainers to trust your patches >>> anymore. >>> >> Hi, >> >> I am quite sorry for my incorrect patches, and I will listen carefully >> to your advice. In fact, for some bugs and patches which I have >> reported before, I have not received the feedback of them, so I resent >> them a few days ago, including this patch. > Yeah, it is likely that some of your reports will not get any response. > For that I only suggest being persistent and providing more information > about the issue and suggestions how it might be possible to fix it. Also > Dan Carpenter (Cced) might have some suggestions. > > But trying to "fix" it by just silencing the warning without proper > analysis is totally the wrong approach, you do more harm than good. > > What tool do you use to find these issues? Is it publically available? >
Hi,
Thanks a lot for your advice. And I am very glad to see that you may be interested in my work :) This static tool is written by myself, instead of using or improving existing tools. A reason why I write it is that I have encountered some sleep-in-atomic bugs in my driver development :( . However, due to preliminary implementation, this tool still has some limitations which can produce some false positives or negatives, and it may be not very easy to use. Thus, I am still improving this tool, checking more code and collecting results now. By the way, I apologize again for my incorrect patches of trying to "fix" the detected bugs. In fact, I am very glad to make this tool available to effectively and conveniently check more system code. After I finish the improvements and perform more evaluation, I will make it publicly available. If you have any suggestion or comment on my work, please feel free to contact me :)
Thanks, Jia-Ju Bai
| |