On Tue, Jun 18, 2019 at 8:54 PM Guoqing Jiang <gqjiang@xxxxxxxx> wrote:
>
>
>
> On 6/18/19 10:39 PM, Song Liu wrote:
> > 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?
>
> Yes, you are right, thanks for the details.
>
> > And we probably over writing one element on wb_list.
>
> Will change it to:
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 93dff41c4ff9..f1ea17b14b6e 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -85,7 +85,7 @@ static void lower_barrier(struct r1conf *conf,
> sector_t sector_nr);
>
> static int check_and_add_wb(struct md_rdev *rdev, sector_t lo,
> sector_t hi)
> {
> - struct wb_info *wi;
> + struct wb_info *wi, *temp_wi;
> unsigned long flags;
> int ret = 0;
> struct mddev *mddev = rdev->mddev;
> @@ -93,9 +93,9 @@ static int check_and_add_wb(struct md_rdev *rdev,
> sector_t lo, sector_t hi)
> 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) {
> + list_for_each_entry(temp_wi, &rdev->wb_list, list) {
> /* collision happened */
> - if (hi > wi->lo && lo < wi->hi) {
> + if (hi > temp_wi->lo && lo < temp_wi->hi) {
> ret = -EBUSY;
> break;
> }
This looks good.
Thanks!
Song
