Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

[Sean Fu]

On Wed, Sep 9, 2015 at 12:36 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Tue, 08 Sep 2015 11:19:14 -0500
> ebiederm@xxxxxxxxxxxx (Eric W. Biederman) wrote:
>
>
>> This patch does not implement the old behavior.
>>
>> The old code does use '\0' as a buffer terminator, and because it does
>> not check things closely I can see how it could accept a '\0' from
>> userspace and treat that as an early buffer terminator.
>>
>> The patch treats '\0' as a number separator and allows things that have
>> never been allowed before and quite frankly is very scary as it just
>> invites bugs.
>>
>> So I do not think we should merge the given patch.  It is just wrong.
>> One that simply truncates the input buffer at the first '\0' character I
>> think we can consider, although I am not a fan.
>
> I agree, and was thinking this patch did that, but I didn't look close
> enough (why I never gave a Reviewed-by to it either).
>
>
>>
>> Steve as far as what I think would break.  I don't think the current
>> behavior should have broken anything and apparently it did.  I don't see
>> what a change that simply truncates the buffer at the first embedded
>> '\0' would break, but I don't know how to test that there isn't anything
>> that it will.  We are way past the point of reasonable expectations
>> being able to guide us.  4 years should have been more than enough soak
>> time to have been able to say that the change was good, but apparently
>> it was not.
>
> Well, to be fair, a lot of people (distros, etc) do not use the most
> recent kernels. 4 years may be the first time a tool touches a kernel.
>
>>
>> My gut feel says that if we are going to change this, at this late date,
>> we find the one specific proc file that matters and change it just for
>> that one proc file, and in that change we treat '\0' as a terminator not
>> as a separator.  I never did see in the conversation which proc file it
>> is that actually matters.  The principle is that the more precise and
>> the more localized such a change is the less chance it has of causing a
>> regression of something else, and the greater the chance we can look at
>> a specific issue.
>
> Sounds like a reasonable compromise. Sean, can you make a patch that
> only affects the one proc file (comment it well in the code), and have
> it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
> would only see "1 "
The current code uses uniform handler (e.g. "proc_dointvec") for all
same type proc file.
So all integer type proc file are affected.
In fact, The behavior of all integer type proc file should be changed.
>
> -- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/