On Tue, Jun 18, 2019 at 1:35 AM Guoqing Jiang <gqjiang@xxxxxxxx> wrote:
>
>
>
> On 6/18/19 11:41 AM, Guoqing Jiang wrote:
> >>>
> >>> +static int check_and_add_wb(struct md_rdev *rdev, sector_t lo,
> >>> sector_t hi)
> >>> +{
> >>> + struct wb_info *wi;
> >>> + unsigned long flags;
> >>> + int ret = 0;
> >>> + struct mddev *mddev = rdev->mddev;
> >>> +
> >>> + wi = mempool_alloc(mddev->wb_info_pool, GFP_NOIO);
> >>> +
> >>> + spin_lock_irqsave(&rdev->wb_list_lock, flags);
> >>> + list_for_each_entry(wi, &rdev->wb_list, list) {
> >> This doesn't look right. We should allocate wi from mempool after
> >> the list_for_each_entry(), right?
> >
> > Good catch, I will use an temp variable in the iteration since
> > mempool_alloc
> > could sleep.
>
> After a second thought, I think it should be fine since wi is either
> reused to record the sector range or freed after the iteration.
>
> Could you elaborate your concern? Thanks.
First, we allocated wb_info and stored the ptr to wi. Then we use
same wi in list_for_each_entry(), where the original value of wi is
overwritten:
#define list_for_each_entry(pos, head, member) \
for (pos = list_first_entry(head, typeof(*pos), member); \
&pos->member != (head); \
pos = list_next_entry(pos, member))
So we are leaking the original wi from mempool_alloc, right?
And we probably over writing one element on wb_list.
Did I read it wrong?
Thanks,
Song
