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

[Eric W. Biederman]

Steven Rostedt <rostedt@xxxxxxxxxxx> writes:

> On Tue, 8 Sep 2015 11:11:38 +0800
> Sean Fu <fxinrong@xxxxxxxxx> wrote:
>
>> On Fri, Aug 28, 2015 at 11:31 AM, Sean Fu <fxinrong@xxxxxxxxx> wrote:
>> > On Thu, Aug 27, 2015 at 4:32 PM, Sean Fu <fxinrong@xxxxxxxxx> wrote:
>> >> On Thu, Aug 27, 2015 at 10:32 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>> >>> On Thu, 27 Aug 2015 08:17:29 +0800
>> >>> Sean Fu <fxinrong@xxxxxxxxx> wrote:
>> >>>> strace execute result:
>> >>>> write(3, "1\2\0", 3)                    = -1 EINVAL (Invalid argument)
>> > If vleft > 1, "1\0 2" is treated as invalid paraments and all string
>> > include '\0' will be invalid.
>> Hi All experts,
>> Could you please signed off this patch?
>
> If anyone should take this, it would be Andrew.
>
> I have no issue with the patch. Eric, you had some issue, but I don't
> see a scenario that would depend on the current behavior. That is, what
> do you think would break if we put it back to the old behavior?

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.

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.

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.

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