On Tue, 19 Dec 2023 11:32:02 +0800, Baoquan He <bhe@xxxxxxxxxx> wrote: > Hi Yuntao, > > On 12/19/23 at 10:02am, Yuntao Wang wrote: > > On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <ytcoode@xxxxxxxxx> wrote: > > > > > > > mem->nr_ranges represents the current number of elements stored in > > > > the mem->ranges array, and mem->max_nr_ranges represents the maximum number > > > > of elements that the mem->ranges array can hold. Therefore, the correct > > > > array out-of-bounds check should be mem->nr_ranges >= mem->max_nr_ranges. > > > > > > > > > > This does not apply after your own "crash_core: fix and simplify the > > > logic of crash_exclude_mem_range()". What should be done? > > > > Hi Andrew, > > > > I actually prefer the "crash_core: fix and simplify the logic of > > crash_exclude_mem_range()" patch as it makes the final code more concise and > > clear, and less prone to errors. > > > > The current code is too strange, I guess no one can understand why there is > > a break in the for loop when they read this code for the first time. > > > > Moreover, I think the current code is too fragile, it relies on callers using > > this function correctly to ensure its correctness, rather than being able to > > guarantee the correctness on its own. I even feel that this function is very > > likely to have bugs again as the code evolves. > > > > However, Baoquan also has his own considerations, he suggests keeping the code > > as it is. > > > > The link below is our detailed discussion on this issue: > > There's misunderstanding here. > > Firstly I said I have concern about the patch, I didn't NACK or reject the patch. > > [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range() > > Usually, when people said he/she had concern, you may need to > investigate and resolve it or explain why it's not need be cared about. > > E.g on above [PATCH 3/3], we can add below code change to stop scanning > when the left ranges are all above the excluded range, assume the passed > in cmem has a ascending order of ranges. Say so because I checked code > and found that crash_exclude_mem_range() is called in arch arm64, ppc, > riscv and x86. Among them, arm64 and ppc create the cmem from memblock, > riscv and x86 create cmem from iomem. All of them should be in ascending > ordr. The below code change based on your patch 3/3 looks safe to me. > What do you think? > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index aab342c2a5ee..39b6c149dc80 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -574,9 +574,12 @@ int crash_exclude_mem_range(struct crash_mem *mem, > p_start = mstart; > p_end = mend; > > - if (p_start > end || p_end < start) > + if (p_start > end) > continue; > > + if (p_end < start) > + break; > + > /* Truncate any area outside of range */ > if (p_start < start) > p_start = start; > > Secondly, I welcome people who are interested kexec/kdump code, and raise > issues or post patches to fix bug, clean up code. I like these patches. > They can help improve kexec/kdump code and solve problem in advance. > I would like to review and make the patches acceptable and merged > inally. And I also hope people can follow the later issue reported by > other people or LKP if their merged patch caused that. > > Lastly, people are encouraged to help review other people's patch > and give suggestes to improve the code change. If patch author don't > respond for a long while or the work has been suspended for long time, we > can add comment to tell and take over the work to continue. > > These are my personal understanding and thought about kexec/kdump patch > reviewing and maintance. So cheer up. > > > > > https://lore.kernel.org/lkml/20231214163842.129139-3-ytcoode@xxxxxxxxx/t/#mfd78a97e16251bcb190b0957a0b6cb4b0a096b54 > > > > The final decision on whether to apply that patch is up to you and Baoquan, if > > you choose to apply that patch, this patch can be ignored. But if you decide not > > to apply that patch, then this patch must be applied, as it fixes a bug in the > > crash_exclude_mem_range() function. > > > > Sincerely, > > Yuntao Hi Baoquan, I must clarify that I was not complaining about you. On the contrary, I am grateful to everyone who takes time to review code for others, because I know it is a lot of work. I'm relatively new to the Linux community and still learning the various rules of the community. I'm very sorry that I didn't fully grasp your previous intention. Regarding the method you suggested to add a 'break', I did consider it initially but later decided against it because the memory ranges obtained from iomem may overlap, so I chose a safer way instead. Finally, I would like to apologize again if my previous response offended you. That was not my intention. Sincerely, Yuntao _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
