Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: VwTqOj2jPLcowky/2JJ09tQO6hG1v9cFL/rs2Wl/m4iXIagaqsypRDWkUIYwSaGMrNpTEHt9xJ
 GOeZBFAnKerA==
IronPort-SDR: o7gO3hg17ryKJug7bQpOb35jPcYNb2P308g6e0B8Bqy4Yi0NV3vsp6mEkAAEJIbLnhpdHPGjng
 5VAC4qaBV8Vg==
Subject: Re: [PATCH v2] dmaengine: check device and channel list for empty
To:     Jiri Slaby <jirislaby@kernel.org>, vkoul@kernel.org
Cc:     Swathi Kovvuri <swathi.kovvuri@intel.com>, peter.ujfalusi@ti.com,
        Linux kernel mailing list <linux-kernel@vger.kernel.org>
References: <159319496403.69045.16298280729899651363.stgit@djiang5-desk3.ch.intel.com>
 <ea3ef860-0b7a-e8da-8cf9-5930a8f3b7ed@kernel.org>
 <b9e8f171-6961-b483-c698-18a89e58f361@intel.com>
 <852318ec-9e18-3dee-a91d-1cf4dddb8906@kernel.org>
From:   Dave Jiang <dave.jiang@intel.com>
Message-ID: <83932426-d52a-2e62-9d4b-5abb134a64df@intel.com>
Date:   Thu, 9 Jul 2020 08:23:41 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <852318ec-9e18-3dee-a91d-1cf4dddb8906@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 7/8/2020 10:35 PM, Jiri Slaby wrote:
> On 07. 07. 20, 17:42, Dave Jiang wrote:
>> On 7/6/2020 11:05 PM, Jiri Slaby wrote:
>>> On 26. 06. 20, 20:09, Dave Jiang wrote:
>>>> Check dma device list and channel list for empty before iterate as the
>>>> iteration function assume the list to be not empty. With devices and
>>>> channels now being hot pluggable this is a condition that needs to be
>>>> checked. Otherwise it can cause the iterator to spin forever.
>>>
>>> Could you be a little bit more specific how this can spin forever? I.e.
>>> can you attach a stacktrace of such a behaviour?
>>
>> I can't seem to find the original splat that lead me to the conclusion
>> of it's spinning forever. As I recall, the issue seems to produce
>> different splats and not always consistent in being reproduced. Here's a
>> partial splat that was tracked by the internal bug database. Since with
>> the dma device and channel list being are hot added and removed, the
>> device and channel lists can be empty. The list_entry() and friends
>> expect the list to not be empty (according to header comment), I added
>> the check to ensure that isn't the case before using them in dmaengine.
> 
> Yes, the comment states that as it is true: you receive a
> wild/non-checkable pointer if you do list_entry on an empty list. BUT
> have you actually read what I wrote:
> 
>>> As in the empty case, "&pos->member" is "head" (look into
>>> list_for_each_entry) and the for loop should loop exactly zero times.
> 
> HERE ^^^^
> 
>> With the fix, we can no longer produce any of the splats. So maybe the
>> above was a bad description of the issue.
> 
> No, not only the description, worse, the patch proper looks wrong.
> 
>> [ 4216.048375]  ? dma_channel_rebalance+0x7b/0x250
>> [ 4216.056360]  dma_async_device_register+0x349/0x3a0
>> [ 4216.064604]  idxd_register_dma_device+0x90/0xc0 [idxd]
>> [ 4216.073175]  idxd_config_bus_probe.cold+0x7d/0x1fc [idxd]
> 
> So, the good part in the patch is the fixed locking in
> dma_async_device_register. Otherwise it adds nonsense checks. So you
> fixed the issue only by a chance, by a side effect as Peter pointed out.
> Leaving aside that you broke dma_request_chan -- that could happen to
> anybody.
> 
> Vinod, please drop/revert this patch. Then start over only with
> dma_async_device_register fixed locking.

I'll start on the proper fix.

> 
> thanks,
>