Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a22358ce-8eb3-cd34-47e1-363ce37e0ae7@windriver.com>
Date:   Sat, 20 Aug 2022 08:24:52 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [V2][PATCH] asm-generic: sections: refactor memory_intersects
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Thierry Reding <treding@nvidia.com>,
        Ard Biesheuvel <ardb@kernel.org>, linux-arch@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220819081145.948016-1-quanyang.wang@windriver.com>
 <20220819145100.ad9d08094ab9f345563fc52b@linux-foundation.org>
From:   quanyang wang <quanyang.wang@windriver.com>
In-Reply-To: <20220819145100.ad9d08094ab9f345563fc52b@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZU51ejN4ZTFoSUFlWHZXRjYvL1h4VDdmUTJTNEs1VEkwVnVOemNZdzdpcFlC?=
 =?utf-8?B?MGwxNDVqb1ozNGZCZ1E2b0UrT0RUVWtNUEtUUVRzTFVwNk10YldEZjkxZWQ3?=
 =?utf-8?B?WExNLzJZNGtRMjVJZUVqSWliSzFISkkycStOQkI1R01FWThqVnd0b2tRSmw3?=
 =?utf-8?B?d1VFcU5qWUpNTTgyc092R2hoRlFpaE1jUWVzdGVUMU90SXZQR3Bka0ZtcVlH?=
 =?utf-8?B?S1hiR1R2WDcrUVVOMkF1TFNOMDZRNzFjM2xzVEt0dGtVUXk3NkxXaGp1dEpT?=
 =?utf-8?B?UmxEcjBiR0hYVUVXVVJ2SlBxVklobEYzM08xaHZxbFpxMlp3WXlOTHNObFFO?=
 =?utf-8?B?cDhHSTBOR0RBajZyMkduTHhIbmFGZ0FkV2JBbVRZTmxRQkhENnBjbFdkc01q?=
 =?utf-8?B?djUrUzJSZlV6c2VEZWUweHhOclV5MmU1dFluWE92RVdaUTJoSUtGNFZJeGVl?=
 =?utf-8?B?RjJ6TExZQlhHc3BQL0dreDB1UjJZWlBjS29CYStvblhwOExvN2ZKSlRiVXFo?=
 =?utf-8?B?RDlQaE1MVTl3VHcvc0o5M01Ob2tqb0NhbkZiN2hhaG5kZVB4SnV6RGtWS2J6?=
 =?utf-8?B?RGdtSjU5U3dMQU9RdEYwYUdOak5GMGFOZ0txM08xNDlYSDRraXBnUG1ndW1x?=
 =?utf-8?B?T1duektKRU15Lzd4RHFYTHN2Ym9tS2hrWWd0VWJxVC9JN3FTczU4ZzhpVWh0?=
 =?utf-8?B?a0xZZFVPRVN5aHVvRDRKSkVVS3hTaGtNalpUa1U1Vmh2Q2ZRZWJUcDh2R0lm?=
 =?utf-8?B?NHFvVlBkaTdPOVZ1Ym45b3hOdnhQekhYSCt6TmtNYlFoS2IwKzViOG1oNGpK?=
 =?utf-8?B?eE8yZEtSUldkZm9NckdoU3FIR09La0FFR2NrTlVieDBxTkRhU0M3T1JaYVov?=
 =?utf-8?B?WTRKRUU1SmtqN1RVS3lwRitLcWY5bmdtN2NEbnFpcFVBNnp3Z0dBRWFHYWdL?=
 =?utf-8?B?NTFDMTU5S21DdXY3YkxqNllvQ1lxUEVrZTNidTZheElJbjVqN0V5bWlMQ2hp?=
 =?utf-8?B?L3FzeVBkdVV1NTVJaGMxdDZUaCtGeHFkSkUzTzVTZTVWV1hIWmZSQkRpY1JF?=
 =?utf-8?B?MC9pZUpSVkxxaFFkRmkvajJLQ3pQb0lLd0UzeFl1cUxwK0RBYXBmU0s1bC9L?=
 =?utf-8?B?QkllTGpRcGVMKzh1ZzRNK3Z4SGRIUWlFZ3A1Rm92VVZlSDlyVE1HRWl3Zlgx?=
 =?utf-8?B?UlNJbmMzOXBUcWxLcnplcGViVDkwVC9pUnVzUGlhblVtU0t0M2VlcGZ2QTdt?=
 =?utf-8?B?M01RYkxFeUcweTdPT3g2c1h5SWQxM3lOZit2ZHZsQ1BLLy9VQ0Y5ZW5EQ01W?=
 =?utf-8?B?ZEhDUklRU1d0MzAxZGtuclF5Szd3NFBrT2FKbGdRektSY1RUSGMweU9BU2FT?=
 =?utf-8?B?c3F2WlloVnkyNWp3b3FDcHM4TUVTeFJqdjV6S3A3eDkwb0c0c2E5aWVaTUY2?=
 =?utf-8?B?OUhQRmZsS2NJZzVoYTNBcndUYkt1MUgxSzRTRHY3SmgrbFRsSVBadHprdDkx?=
 =?utf-8?B?SWhsSmtWWkludTdLdGNqd1luenEycGlISkJDMTJkTkJLckZKWGs1NUxmSys2?=
 =?utf-8?B?emVRcHQvaHdaVDRWWXQzR2hwbm5RaVY2djl5RkY1SE1vNkw0ZndsZGU0bmto?=
 =?utf-8?B?bFFMVWlYNEVGRW9IS2dMV0VWTTFSVjYrdG1Ib3hqYVdsemNqY1greWJtNjJu?=
 =?utf-8?B?SUFYZGNoMDZ5ZUxMejRrclR6Nm56QVludDJ0SVNOTUJIYUFFM3RvOWdrcVpv?=
 =?utf-8?B?ZlZtSC9OcUs4WDI2eHZJQ0U5QzBuNkdGcEdrTDFDZjNUbmJrdHV0SmZBVGVB?=
 =?utf-8?B?SitlRm1WNWdUTkhLWmlnQkJlbWdtUkRpRlhIKzZweFViSGQ0UHRwM3dLT0Ur?=
 =?utf-8?B?TlpNZnhYMkFVb0thK1g2a25qdDY1M0xNQWZDWUhYVVZ5Nm5JZCtUNjJKOXl3?=
 =?utf-8?B?dmRKMEFPcDBJUHk2Wi9OakxFdTluSGJTZ1VPOGR5b1hBYnN1Vk1tc2QvOFVU?=
 =?utf-8?B?MHMzZm9QY1N0RmV5Y3lLWUtxMWx6WFVKUndRbnB1bzVqZCs1S3hSWktlZ1pt?=
 =?utf-8?B?Nzd2aitmVzB0cUxWNmhISkd1Mi9wRktOWnFyQ0lyRStXL3pLYlFJM1oydkZy?=
 =?utf-8?B?MVBvNzNXbHVoNXBGM3BNZmpkcFdCSkhYUFBWRmx3NW9GZUdSSG1pdFN2TWh5?=
 =?utf-8?B?a2c9PQ==?=
X-OriginatorOrg: windriver.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a06ac67a-3281-4190-6013-08da82427090
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5630.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Aug 2022 00:25:09.7276
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ddb2873-a1ad-4a18-ae4e-4644631433be
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /BcFyVHA2ImwlJW5FMo30BHpriHLPraR3TKBDVZgCyaF/05jxsJfOfK8BmWNL8vJ96rWGP9FX5MCp1+wv1JXzXtdJN0zadyLkthaQDIx7qI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB3822
X-Proofpoint-ORIG-GUID: dezW8FopMotSvr1nl6svoLFK0Frjq8dX
X-Proofpoint-GUID: dezW8FopMotSvr1nl6svoLFK0Frjq8dX
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-19_13,2022-08-18_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 bulkscore=0
 spamscore=0 impostorscore=0 suspectscore=0 phishscore=0 clxscore=1015
 lowpriorityscore=0 mlxscore=0 adultscore=0 priorityscore=1501
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2207270000 definitions=main-2208190091
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andrew,

On 2022/8/20 05:51, Andrew Morton wrote:
> On Fri, 19 Aug 2022 16:11:45 +0800 quanyang.wang@windriver.com wrote:
>
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> There are two problems with the current code of memory_intersects:
>>
>> First, it doesn't check whether the region (begin, end) falls inside
>> the region (virt, vend), that is (virt < begin && vend > end).
>>
>> The second problem is if vend is equal to begin, it will return true
>> but this is wrong since vend (virt + size) is not the last address of
>> the memory region but (virt + size -1) is. The wrong determination will
>> trigger the misreporting when the function check_for_illegal_area calls
>> memory_intersects to check if the dma region intersects with stext region.
>>
>> The misreporting is as below (stext is at 0x80100000):
>>   WARNING: CPU: 0 PID: 77 at kernel/dma/debug.c:1073 check_for_illegal_area+0x130/0x168
>>   DMA-API: chipidea-usb2 e0002000.usb: device driver maps memory from kernel text or rodata [addr=800f0000] [len=65536]
>>   Modules linked in:
>>   CPU: 1 PID: 77 Comm: usb-storage Not tainted 5.19.0-yocto-standard #5
>>   Hardware name: Xilinx Zynq Platform
>>    unwind_backtrace from show_stack+0x18/0x1c
>>    show_stack from dump_stack_lvl+0x58/0x70
>>    dump_stack_lvl from __warn+0xb0/0x198
>>    __warn from warn_slowpath_fmt+0x80/0xb4
>>    warn_slowpath_fmt from check_for_illegal_area+0x130/0x168
>>    check_for_illegal_area from debug_dma_map_sg+0x94/0x368
>>    debug_dma_map_sg from __dma_map_sg_attrs+0x114/0x128
>>    __dma_map_sg_attrs from dma_map_sg_attrs+0x18/0x24
>>    dma_map_sg_attrs from usb_hcd_map_urb_for_dma+0x250/0x3b4
>>    usb_hcd_map_urb_for_dma from usb_hcd_submit_urb+0x194/0x214
>>    usb_hcd_submit_urb from usb_sg_wait+0xa4/0x118
>>    usb_sg_wait from usb_stor_bulk_transfer_sglist+0xa0/0xec
>>    usb_stor_bulk_transfer_sglist from usb_stor_bulk_srb+0x38/0x70
>>    usb_stor_bulk_srb from usb_stor_Bulk_transport+0x150/0x360
>>    usb_stor_Bulk_transport from usb_stor_invoke_transport+0x38/0x440
>>    usb_stor_invoke_transport from usb_stor_control_thread+0x1e0/0x238
>>    usb_stor_control_thread from kthread+0xf8/0x104
>>    kthread from ret_from_fork+0x14/0x2c
>>
>> Refactor memory_intersects to fix the two problems above.
>>
>> ...
> There must be tons of places in the kernel which check to see if two
> regions overlap at all, I'm not sure why dma debug needs its own one?
>
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -110,7 +110,10 @@ static inline bool memory_intersects(void *begin, void *end, void *virt,
>>   {
>>   	void *vend = virt + size;
>>   
>> -	return (virt >= begin && virt < end) || (vend >= begin && vend < end);
>> +	if (virt < end && vend > begin)
>> +		return true;
>> +
>> +	return false;
>>   }
> These things bend my brain, but all the cases I've mind-tested worked
> out OK.
>
> Now the forever question: is a -stable backport needed?  The bug
> appears to be six years old, so I guess not.  Can you suggest why it
> took this long?  Are you doing something unusual?

Before the commit 1d7db834a027e ("dma-debug: use memory_intersects() 
directly") , memory_intersects is called only by printk_late_init:

printk_late_init -> init_section_intersects ->memory_intersects.

There are few places memory_intersects is called.

When the commit 1d7db834a027e ("dma-debug: use memory_intersects() 
directly") is merged and CONFIG_DMA_API_DEBUG is enabled,

DMA subsystem uses it to check illegal area and trigger the calltrace above.

Thanks,

Quanyang

>
>
> While we're in there, I can't resist fixing that typo...
>
> --- a/include/asm-generic/sections.h~asm-generic-sections-refactor-memory_intersects-fix
> +++ a/include/asm-generic/sections.h
> @@ -97,7 +97,7 @@ static inline bool memory_contains(void
>   /**
>    * memory_intersects - checks if the region occupied by an object intersects
>    *                     with another memory region
> - * @begin: virtual address of the beginning of the memory regien
> + * @begin: virtual address of the beginning of the memory region
>    * @end: virtual address of the end of the memory region
>    * @virt: virtual address of the memory object
>    * @size: size of the memory object
> _
>