| Message ID | 1519468406-67354-1-git-send-email-cgxu519@icloud.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 2/24/18 4:33 AM, Chengguang Xu wrote: > When specifying string type mount option (e.g., logdev) > several times in a mount, current option parsing may > cause memory leak. Hence, call kfree for previous one > in this case. > > Signed-off-by: Chengguang Xu <cgxu519@icloud.com> IMHO multiple specifications of logdev or rtdev should not be allowed; only one may be chosen, and specifying twice should probably be considered and administrative error. I'd rather see something that fails and cleans up properly if either option is respecified. Thanks, -Eric > --- > fs/xfs/xfs_super.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 7aba628..93588ea 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -250,6 +250,7 @@ enum { > return -EINVAL; > break; > case Opt_logdev: > + kfree(mp->m_logname); > mp->m_logname = match_strdup(args); > if (!mp->m_logname) > return -ENOMEM; > @@ -258,6 +259,7 @@ enum { > xfs_warn(mp, "%s option not allowed on this system", p); > return -EINVAL; > case Opt_rtdev: > + kfree(mp->m_rtname); > mp->m_rtname = match_strdup(args); > if (!mp->m_rtname) > return -ENOMEM; > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： > > On 2/24/18 4:33 AM, Chengguang Xu wrote: >> When specifying string type mount option (e.g., logdev) >> several times in a mount, current option parsing may >> cause memory leak. Hence, call kfree for previous one >> in this case. >> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > > IMHO multiple specifications of logdev or rtdev should not > be allowed; only one may be chosen, and specifying twice > should probably be considered and administrative error. > > I'd rather see something that fails and cleans up properly > if either option is respecified. Hi Eric, Thanks for quick reply. I think your suggestion makes sense, but considering of the consistency with other options, we should check multiple specifications for all options. So I think this patch is the cheapest way to fix the issue. What do you think? Thanks, Chengguang.-- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 12:11:23PM +0800, Chengguang Xu wrote: > > > > 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： > > > > On 2/24/18 4:33 AM, Chengguang Xu wrote: > >> When specifying string type mount option (e.g., logdev) > >> several times in a mount, current option parsing may > >> cause memory leak. Hence, call kfree for previous one > >> in this case. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > > > > IMHO multiple specifications of logdev or rtdev should not > > be allowed; only one may be chosen, and specifying twice > > should probably be considered and administrative error. > > > > I'd rather see something that fails and cleans up properly > > if either option is respecified. > > Hi Eric, > > Thanks for quick reply. I think your suggestion makes sense, > but considering of the consistency with other options, > we should check multiple specifications for all options. > > So I think this patch is the cheapest way to fix the issue. > What do you think? I think the all the mount options should be cleaned up to allow one specification only. --D > > > Thanks, > Chengguang.-- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/25/18 12:09 PM, Darrick J. Wong wrote: > On Sun, Feb 25, 2018 at 12:11:23PM +0800, Chengguang Xu wrote: >>> >>> 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： >>> >>> On 2/24/18 4:33 AM, Chengguang Xu wrote: >>>> When specifying string type mount option (e.g., logdev) >>>> several times in a mount, current option parsing may >>>> cause memory leak. Hence, call kfree for previous one >>>> in this case. >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>> >>> IMHO multiple specifications of logdev or rtdev should not >>> be allowed; only one may be chosen, and specifying twice >>> should probably be considered and administrative error. >>> >>> I'd rather see something that fails and cleans up properly >>> if either option is respecified. >> >> Hi Eric, >> >> Thanks for quick reply. I think your suggestion makes sense, >> but considering of the consistency with other options, >> we should check multiple specifications for all options. >> >> So I think this patch is the cheapest way to fix the issue. >> What do you think? > > I think the all the mount options should be cleaned up to allow one > specification only. Yeah, I think so too, though perhaps it's fair to have one patch to fix the leak as it stands today, and another to clean up the respecification problem. They are two separate issues in the end, I suppose, so two separate patches seems reasonable. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 25, 2018 at 12:11:28PM -0600, Eric Sandeen wrote: > > > On 2/25/18 12:09 PM, Darrick J. Wong wrote: > > On Sun, Feb 25, 2018 at 12:11:23PM +0800, Chengguang Xu wrote: > >>> > >>> 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： > >>> > >>> On 2/24/18 4:33 AM, Chengguang Xu wrote: > >>>> When specifying string type mount option (e.g., logdev) > >>>> several times in a mount, current option parsing may > >>>> cause memory leak. Hence, call kfree for previous one > >>>> in this case. > >>>> > >>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > >>> > >>> IMHO multiple specifications of logdev or rtdev should not > >>> be allowed; only one may be chosen, and specifying twice > >>> should probably be considered and administrative error. > >>> > >>> I'd rather see something that fails and cleans up properly > >>> if either option is respecified. > >> > >> Hi Eric, > >> > >> Thanks for quick reply. I think your suggestion makes sense, > >> but considering of the consistency with other options, > >> we should check multiple specifications for all options. > >> > >> So I think this patch is the cheapest way to fix the issue. > >> What do you think? > > > > I think the all the mount options should be cleaned up to allow one > > specification only. > > Yeah, I think so too, though perhaps it's fair to have one patch > to fix the leak as it stands today, and another to clean up the > respecification problem. Won't that break existing userspace configs? Haven't we had cases in the past where mount did something whacky and passed dulicate options to the kernel through no fault of the user? Cheers, Dave.
> On Feb 25, 2018, at 3:05 PM, Dave Chinner <david@fromorbit.com> wrote: > >> On Sun, Feb 25, 2018 at 12:11:28PM -0600, Eric Sandeen wrote: >> >> >>> On 2/25/18 12:09 PM, Darrick J. Wong wrote: >>> On Sun, Feb 25, 2018 at 12:11:23PM +0800, Chengguang Xu wrote: >>>>> >>>>> 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： >>>>> >>>>>> On 2/24/18 4:33 AM, Chengguang Xu wrote: >>>>>> When specifying string type mount option (e.g., logdev) >>>>>> several times in a mount, current option parsing may >>>>>> cause memory leak. Hence, call kfree for previous one >>>>>> in this case. >>>>>> >>>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>>>> >>>>> IMHO multiple specifications of logdev or rtdev should not >>>>> be allowed; only one may be chosen, and specifying twice >>>>> should probably be considered and administrative error. >>>>> >>>>> I'd rather see something that fails and cleans up properly >>>>> if either option is respecified. >>>> >>>> Hi Eric, >>>> >>>> Thanks for quick reply. I think your suggestion makes sense, >>>> but considering of the consistency with other options, >>>> we should check multiple specifications for all options. >>>> >>>> So I think this patch is the cheapest way to fix the issue. >>>> What do you think? >>> >>> I think the all the mount options should be cleaned up to allow one >>> specification only. >> >> Yeah, I think so too, though perhaps it's fair to have one patch >> to fix the leak as it stands today, and another to clean up the >> respecification problem. > > Won't that break existing userspace configs? Haven't we had cases in > the past where mount did something whacky and passed dulicate > options to the kernel through no fault of the user? > Hmm perhaps on remount yes... worth testing / checking. Eric > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 在 2018年2月26日，上午2:11，Eric Sandeen <sandeen@sandeen.net> 写道： > > > > On 2/25/18 12:09 PM, Darrick J. Wong wrote: >> On Sun, Feb 25, 2018 at 12:11:23PM +0800, Chengguang Xu wrote: >>>> >>>> 在 2018年2月25日，上午1:58，Eric Sandeen <sandeen@sandeen.net> 写道： >>>> >>>> On 2/24/18 4:33 AM, Chengguang Xu wrote: >>>>> When specifying string type mount option (e.g., logdev) >>>>> several times in a mount, current option parsing may >>>>> cause memory leak. Hence, call kfree for previous one >>>>> in this case. >>>>> >>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>>> >>>> IMHO multiple specifications of logdev or rtdev should not >>>> be allowed; only one may be chosen, and specifying twice >>>> should probably be considered and administrative error. >>>> >>>> I'd rather see something that fails and cleans up properly >>>> if either option is respecified. >>> >>> Hi Eric, >>> >>> Thanks for quick reply. I think your suggestion makes sense, >>> but considering of the consistency with other options, >>> we should check multiple specifications for all options. >>> >>> So I think this patch is the cheapest way to fix the issue. >>> What do you think? >> >> I think the all the mount options should be cleaned up to allow one >> specification only. > > Yeah, I think so too, though perhaps it's fair to have one patch > to fix the leak as it stands today, and another to clean up the > respecification problem. They are two separate issues in the end, > I suppose, so two separate patches seems reasonable. More accurately, we don’t allow respecification with different value, right? Thanks, Chengguang.-- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/24/18 4:33 AM, Chengguang Xu wrote: > When specifying string type mount option (e.g., logdev) > several times in a mount, current option parsing may > cause memory leak. Hence, call kfree for previous one > in this case. > > Signed-off-by: Chengguang Xu <cgxu519@icloud.com> It might be worth investigating whether option respecification should be allowed, but does fix the immediate problem cleanly, so: Reviewed-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/xfs/xfs_super.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 7aba628..93588ea 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -250,6 +250,7 @@ enum { > return -EINVAL; > break; > case Opt_logdev: > + kfree(mp->m_logname); > mp->m_logname = match_strdup(args); > if (!mp->m_logname) > return -ENOMEM; > @@ -258,6 +259,7 @@ enum { > xfs_warn(mp, "%s option not allowed on this system", p); > return -EINVAL; > case Opt_rtdev: > + kfree(mp->m_rtname); > mp->m_rtname = match_strdup(args); > if (!mp->m_rtname) > return -ENOMEM; > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 7aba628..93588ea 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -250,6 +250,7 @@ enum { return -EINVAL; break; case Opt_logdev: + kfree(mp->m_logname); mp->m_logname = match_strdup(args); if (!mp->m_logname) return -ENOMEM; @@ -258,6 +259,7 @@ enum { xfs_warn(mp, "%s option not allowed on this system", p); return -EINVAL; case Opt_rtdev: + kfree(mp->m_rtname); mp->m_rtname = match_strdup(args); if (!mp->m_rtname) return -ENOMEM;
When specifying string type mount option (e.g., logdev) several times in a mount, current option parsing may cause memory leak. Hence, call kfree for previous one in this case. Signed-off-by: Chengguang Xu <cgxu519@icloud.com> --- fs/xfs/xfs_super.c | 2 ++ 1 file changed, 2 insertions(+)