| Message ID | 20180525030547.23367-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 25, 2018 at 11:05:47AM +0800, Anand Jain wrote: > Balance arg info is an important information to be reviewed for the > system audit. So this patch adds them to the kernel log. > > Example: > ->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs > > kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v4->v4.1b: Rename last missed sz to size_buf. Please send a full version instead of the incremental bumps to individual patches. I try to take the last version but am never sure what exactly gets merged. > +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf, > + u32 size_buf) > +{ > + char *bp = buf; > + u64 flags = bargs->flags; > + char tmp_buf[128]; > + > + if (!flags) > + return 0; > + > + if (flags & BTRFS_BALANCE_ARGS_SOFT) > + bp += snprintf(bp, buf - bp + size_buf, "soft,"); I have some suspicion that this snprintf conscturct is not safe when the value of 'buf - bp + size_buf' becomes accidentally negative. A quick test showed that a sequence of snprintf and incremented bp does not stop writing when the size_buf limit is hit and happily overwrites the memory. We'd have to check that the negative value is never passed to snprintf. The patches are potponed until the issue is resolved, either to verify that it's a false alert or the bounds are correctly checked. The bug should not be in the original code as the buffer is known to be large enough for all the printed bg flags. I'm sorry to delay the patches, the log messages are useful, but I want to be sure that there are no random memory overwrites introduced. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/31/2018 05:47 PM, David Sterba wrote: > On Fri, May 25, 2018 at 11:05:47AM +0800, Anand Jain wrote: >> Balance arg info is an important information to be reviewed for the >> system audit. So this patch adds them to the kernel log. >> >> Example: >> ->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs >> >> kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v4->v4.1b: Rename last missed sz to size_buf. > > Please send a full version instead of the incremental bumps to > individual patches. I try to take the last version but am never sure > what exactly gets merged. > >> +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf, >> + u32 size_buf) >> +{ >> + char *bp = buf; >> + u64 flags = bargs->flags; >> + char tmp_buf[128]; >> + >> + if (!flags) >> + return 0; >> + >> + if (flags & BTRFS_BALANCE_ARGS_SOFT) >> + bp += snprintf(bp, buf - bp + size_buf, "soft,"); > > I have some suspicion that this snprintf conscturct is not safe when the > value of 'buf - bp + size_buf' becomes accidentally negative. A quick > test showed that a sequence of snprintf and incremented bp does not stop > writing when the size_buf limit is hit and happily overwrites the > memory. > > We'd have to check that the negative value is never passed to snprintf. > The patches are potponed until the issue is resolved, either to verify > that it's a false alert or the bounds are correctly checked. > > The bug should not be in the original code as the buffer is known to be > large enough for all the printed bg flags. Agreed, possibilities of accidental -ve value for snprintf. It followed the original code, but as you said the data length is more deterministic there. I am fixing this in both new and old original code. Pls find v5. Sorry for the delay, I took some time to get a fresh look at it. Thanks, Anand > I'm sorry to delay the patches, the log messages are useful, but I want > to be sure that there are no random memory overwrites introduced. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index e858882a98eb..c02a4fb8ae7e 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -3809,6 +3809,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg, (bctl_arg->target & ~allowed))); } +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf, + u32 size_buf) +{ + char *bp = buf; + u64 flags = bargs->flags; + char tmp_buf[128]; + + if (!flags) + return 0; + + if (flags & BTRFS_BALANCE_ARGS_SOFT) + bp += snprintf(bp, buf - bp + size_buf, "soft,"); + + if (flags & BTRFS_BALANCE_ARGS_PROFILES) { + btrfs_describe_block_groups(bargs->profiles, tmp_buf, + sizeof(tmp_buf)); + bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,", + tmp_buf); + } + + if (flags & BTRFS_BALANCE_ARGS_USAGE) + bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,", + bargs->usage); + + if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE) + bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,", + bargs->usage_min, bargs->usage_max); + + if (flags & BTRFS_BALANCE_ARGS_DEVID) + bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,", + bargs->devid); + + if (flags & BTRFS_BALANCE_ARGS_DRANGE) + bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,", + bargs->pstart, bargs->pend); + + if (flags & BTRFS_BALANCE_ARGS_VRANGE) + bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,", + bargs->vstart, bargs->vend); + + if (flags & BTRFS_BALANCE_ARGS_LIMIT) + bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,", + bargs->limit); + + if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE) + bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,", + bargs->limit_min, bargs->limit_max); + + if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE) + bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,", + bargs->stripes_min, bargs->stripes_max); + + if (flags & BTRFS_BALANCE_ARGS_CONVERT) { + int index = btrfs_bg_flags_to_raid_index(bargs->target); + + bp += snprintf(bp, buf - bp + size_buf, "convert=%s,", + get_raid_name(index)); + } + + buf[bp - buf - 1] = '\0'; /* remove last , */ + return bp - buf - 1; +} + +static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info) +{ + u32 size_buf = 1024; + char tmp_buf[192]; + char *buf; + char *bp; + struct btrfs_balance_control *bctl = fs_info->balance_ctl; + + buf = kzalloc(size_buf, GFP_KERNEL); + if (!buf) + return; + + bp = buf; + if (bctl->flags & BTRFS_BALANCE_FORCE) + bp += snprintf(bp, buf - bp + size_buf, "-f "); + + if (bctl->flags & BTRFS_BALANCE_DATA) { + describe_balance_args(&bctl->data, tmp_buf, sizeof(tmp_buf)); + bp += snprintf(bp, buf - bp + size_buf, "-d%s ", tmp_buf); + } + + if (bctl->flags & BTRFS_BALANCE_METADATA) { + describe_balance_args(&bctl->meta, tmp_buf, sizeof(tmp_buf)); + bp += snprintf(bp, buf - bp + size_buf, "-m%s ", tmp_buf); + } + + if (bctl->flags & BTRFS_BALANCE_SYSTEM) { + describe_balance_args(&bctl->sys, tmp_buf, sizeof(tmp_buf)); + bp += snprintf(bp, buf - bp + size_buf, "-s%s ", tmp_buf); + } + + buf[bp - buf - 1] = '\0'; /* remove last " "*/ + btrfs_info(fs_info, "%s %s", + bctl->flags & BTRFS_BALANCE_RESUME ? + "balance: resume":"balance: start", buf); + + kfree(buf); +} + /* * Should be called with balance mutexe held */ @@ -3953,6 +4055,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info, ASSERT(!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)); set_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags); + describe_balance_start_or_resume(fs_info); mutex_unlock(&fs_info->balance_mutex); ret = __btrfs_balance(fs_info); @@ -3990,10 +4093,8 @@ static int balance_kthread(void *data) int ret = 0; mutex_lock(&fs_info->balance_mutex); - if (fs_info->balance_ctl) { - btrfs_info(fs_info, "balance: resuming"); + if (fs_info->balance_ctl) ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL); - } mutex_unlock(&fs_info->balance_mutex); return ret;
Balance arg info is an important information to be reviewed for the system audit. So this patch adds them to the kernel log. Example: ->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v4->v4.1b: Rename last missed sz to size_buf. Allocate at least (64*3) 192 bytes for tmp_buf. Log format change: No space after group type, as in the above Example. v3->v4: Change log updated with new example. Log format is changed to almost match with the cli. snprintf drop %s for inline string. Print range as =%u..%u instead of min=%umax=%u. soft is under the args now. force is printed as -f. v2->v3: Change log updated. Make use of describe_block_group() added in 1/4 Drop using strcat instead use snprintf. Logs string format updated as shown in the example above. v1->v2: Change log update. Move adding the logs for balance complete and end to a new patch fs/btrfs/volumes.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 3 deletions(-)