[PATCH] xfs: fix potential memory leak in mount option parsing

[PATCH] xfs: fix potential memory leak in mount option parsing

[Chengguang Xu]

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(+)

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;
-- 
1.8.3.1

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Eric Sandeen]

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;
> 

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Chengguang Xu]

> 
> 在 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.

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Darrick J. Wong]

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

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Eric Sandeen]

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

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Eric Sandeen]

> 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
> 

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Chengguang Xu]

> 
> 在 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.

Re: [PATCH] xfs: fix potential memory leak in mount option parsing

[Eric Sandeen]

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;
>