LinuxLists.cc - [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Fri, Apr 17, 2015 at 10:13:15AM +0800, Wang Xiaoming wrote:
> Move debugging has been done and the following Kernel issue
> was found with a number of applications.
> Take a look at: (even though the comments are for Weibo.browser
> they also pertain to other apps that use Libsecuritysdk-x.x.x.so
>
> In kernel(3.14) is a little different than before
> it will generate /proc/PID/status in this way:
> Name: a.weibo.browser
> State: T (stopped)
> Tgid: 8487
> Ngid: 0 ---- add in kernel after (3.11 maybe)

Well, that's kinda hilarious and I don't know. 3.11 is way back and
what if there are others depending on the current ordering? Both
situations kinda suck so what's the point of changing?

--
tejun

2015-04-17 03:15:49

[permalink] [raw]

Subject: RE: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Dear tejun

> -----Original Message-----
> From: Tejun Heo [mailto:[email protected]] On Behalf Of Tejun Heo
> Sent: Friday, April 17, 2015 10:56 AM
> To: Wang, Xiaoming
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Schallberger, Timothy M;
> Zhang, Dongxing
> Subject: Re: [PATCH] proc: move the adding option Ngid to the end of
> proc/PID/status
>
> On Fri, Apr 17, 2015 at 10:13:15AM +0800, Wang Xiaoming wrote:
> > Move debugging has been done and the following Kernel issue was found
> > with a number of applications.
> > Take a look at: (even though the comments are for Weibo.browser they
> > also pertain to other apps that use Libsecuritysdk-x.x.x.so
> >
> > In kernel(3.14) is a little different than before it will generate
> > /proc/PID/status in this way:
> > Name: a.weibo.browser
> > State: T (stopped)
> > Tgid: 8487
> > Ngid: 0 ---- add in kernel after (3.11 maybe)
>
> Well, that's kinda hilarious and I don't know. 3.11 is way back and what if there
> are others depending on the current ordering? Both situations kinda suck so
> what's the point of changing?
>
I am not sure exactly which kernel introduced this Ngid.
I check the git and found it added in
commit e29cf08b05dc0b8151d65704d96d525a9e179a6b
And this change has destroyed the order already.
Moving the adding option Ngid to the end of proc/PID/status
is to keep order .
> --
> tejun

2015-04-17 03:26:22

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Hello,

On Thu, Apr 16, 2015 at 11:15 PM, Wang, Xiaoming
<[email protected]> wrote:
> I am not sure exactly which kernel introduced this Ngid.
> I check the git and found it added in
> commit e29cf08b05dc0b8151d65704d96d525a9e179a6b

git describe --contains says 3.13 and it's about 1.5 years ago.

> And this change has destroyed the order already.
> Moving the adding option Ngid to the end of proc/PID/status
> is to keep order .

There isn't any assurance that changing the order won't break a
different set of programs which make different ordering assumptions.
The new order is already out there. This isn't a legal dispute where
the original owner is "right".

--
tejun

2015-04-17 03:39:17

[permalink] [raw]

Subject: RE: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Dear tejun

> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Tejun Heo
> Sent: Friday, April 17, 2015 11:26 AM
> To: Wang, Xiaoming
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Schallberger, Timothy M;
> Zhang, Dongxing
> Subject: Re: [PATCH] proc: move the adding option Ngid to the end of
> proc/PID/status
>
> Hello,
>
> On Thu, Apr 16, 2015 at 11:15 PM, Wang, Xiaoming
> <[email protected]> wrote:
> > I am not sure exactly which kernel introduced this Ngid.
> > I check the git and found it added in
> > commit e29cf08b05dc0b8151d65704d96d525a9e179a6b
>
> git describe --contains says 3.13 and it's about 1.5 years ago.
>
Yes this kernel change is 1.5 years ago.
As we known not all user update the kernel so frequently.
They just use the stable one.
We met this issue when update to 3.13 now.
A lot of application failed to run which run well previously.
Do you have any idea on this issue?
> > And this change has destroyed the order already.
> > Moving the adding option Ngid to the end of proc/PID/status is to keep
> > order .
>
> There isn't any assurance that changing the order won't break a different set of
> programs which make different ordering assumptions.
> The new order is already out there. This isn't a legal dispute where the original
> owner is "right".
>
> --
> tejun
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-04-17 03:42:34

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Thu, Apr 16, 2015 at 11:37 PM, Wang, Xiaoming
<[email protected]> wrote:
>> git describe --contains says 3.13 and it's about 1.5 years ago.
>>
> Yes this kernel change is 1.5 years ago.
> As we known not all user update the kernel so frequently.
> They just use the stable one.
> We met this issue when update to 3.13 now.
> A lot of application failed to run which run well previously.
> Do you have any idea on this issue?

Not really. It's a sucky situation. How many applications are we
talking about? I tried to find information on libsecuritysdk but
couldn't find it anywhere.

--
tejun

2015-04-17 05:37:14

[permalink] [raw]

Subject: RE: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Dear tejun

> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Tejun Heo
> Sent: Friday, April 17, 2015 11:42 AM
> To: Wang, Xiaoming
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Schallberger, Timothy M;
> Zhang, Dongxing
> Subject: Re: [PATCH] proc: move the adding option Ngid to the end of
> proc/PID/status
>
> On Thu, Apr 16, 2015 at 11:37 PM, Wang, Xiaoming
> <[email protected]> wrote:
> >> git describe --contains says 3.13 and it's about 1.5 years ago.
> >>
> > Yes this kernel change is 1.5 years ago.
> > As we known not all user update the kernel so frequently.
> > They just use the stable one.
> > We met this issue when update to 3.13 now.
> > A lot of application failed to run which run well previously.
> > Do you have any idea on this issue?
>
> Not really. It's a sucky situation. How many applications are we talking about? I
> tried to find information on libsecuritysdk but couldn't find it anywhere.
>
This lib libsecuritysdk is included in application.
Taobao, weibo, tmall, alipay, etc
It refer to security .
> --
> tejun
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-04-17 13:23:57

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Tejun Heo wrote:
> On Fri, Apr 17, 2015 at 10:13:15AM +0800, Wang Xiaoming wrote:
> > Move debugging has been done and the following Kernel issue
> > was found with a number of applications.
> > Take a look at: (even though the comments are for Weibo.browser
> > they also pertain to other apps that use Libsecuritysdk-x.x.x.so
> >
> > In kernel(3.14) is a little different than before
> > it will generate /proc/PID/status in this way:
> > Name: a.weibo.browser
> > State: T (stopped)
> > Tgid: 8487
> > Ngid: 0 ---- add in kernel after (3.11 maybe)
>
> Well, that's kinda hilarious and I don't know. 3.11 is way back and
> what if there are others depending on the current ordering? Both
> situations kinda suck so what's the point of changing?

It was demonstrated that Ngid addition as line 4 breaks apps,
but your "what if" remains "what if".

I'd say Ngid should be moved to the end and every new field
must be added to the end from now on, people can't parse
simple file correctly, let's not create problems for them.

2015-04-17 14:26:13

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Hey,

On Fri, Apr 17, 2015 at 04:23:48PM +0300, Alexey Dobriyan wrote:
> It was demonstrated that Ngid addition as line 4 breaks apps,
> but your "what if" remains "what if".
>
> I'd say Ngid should be moved to the end and every new field
> must be added to the end from now on, people can't parse
> simple file correctly, let's not create problems for them.

If this were in -rc or we are only a couple releases out, sure, moving
that to the end would be the right thing to do but that's not the case
and it bothers me that the patch essentially trades in about the same
magnitude of unknown risk. No matter which way you spin it, unknown
risk of similar magnitude is not better than known risk and it's
pretty certain that we'll have all three variants out in the wild for
the foreseeable future.

If this has to happen, it should be moving Ngid right after TracerPid
not at the end of file.

Thanks.

--
tejun

2015-04-17 15:05:59

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Fri, Apr 17, 2015 at 5:26 PM, Tejun Heo <[email protected]> wrote:
> Hey,
>
> On Fri, Apr 17, 2015 at 04:23:48PM +0300, Alexey Dobriyan wrote:
>> It was demonstrated that Ngid addition as line 4 breaks apps,
>> but your "what if" remains "what if".
>>
>> I'd say Ngid should be moved to the end and every new field
>> must be added to the end from now on, people can't parse
>> simple file correctly, let's not create problems for them.
>
> If this were in -rc or we are only a couple releases out, sure, moving
> that to the end would be the right thing to do but that's not the case
> and it bothers me that the patch essentially trades in about the same
> magnitude of unknown risk. No matter which way you spin it, unknown
> risk of similar magnitude is not better than known risk and it's
> pretty certain that we'll have all three variants out in the wild for
> the foreseeable future.
>
> If this has to happen, it should be moving Ngid right after TracerPid
> not at the end of file.

Moving Ngid to the end of file minimizes risk of breakage.
Correctly written code doesn't care.
Code which hardcodes layout won't notice.

It would be OK argument if gentlemen from Intel send "let's futureproof and
move Ngid because someone might depend on exact position" patch.

Primum non nocere.

2015-04-17 15:13:07

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Fri, Apr 17, 2015 at 06:05:55PM +0300, Alexey Dobriyan wrote:
> Moving Ngid to the end of file minimizes risk of breakage.

Hmmm... how so? The only reason for changing the position is because
there's this specific breakage. The goal should be working around
that specific case while keeping the impact minimum on everyone else.
It doesn't matter whether the initial change was good or bad, the
kernel w/ the new layout is already out in the wild and it has been
out there for a while. How is risking changing offsets on most of the
fields on those kernels a good idea? Mimize the changes to work
around the specific case.

> Correctly written code doesn't care.
> Code which hardcodes layout won't notice.

Huh? Code which hardcodes layout since 1.5 years ago will definitely
notice.

> It would be OK argument if gentlemen from Intel send "let's futureproof and
> move Ngid because someone might depend on exact position" patch.
>
> Primum non nocere.

ajlkjaeligjlakd lakjeilgjal flekjfa.

--
tejun

2015-04-21 08:19:50

[permalink] [raw]

Subject: RE: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

Dear tejun

> -----Original Message-----
> From: Tejun Heo [mailto:[email protected]] On Behalf Of Tejun Heo
> Sent: Friday, April 17, 2015 11:13 PM
> To: Alexey Dobriyan
> Cc: Wang, Xiaoming; Linux Kernel; Mel Gorman; Andrew Morton
> Subject: Re: [PATCH] proc: move the adding option Ngid to the end of
> proc/PID/status
>
> On Fri, Apr 17, 2015 at 06:05:55PM +0300, Alexey Dobriyan wrote:
> > Moving Ngid to the end of file minimizes risk of breakage.
>
> Hmmm... how so? The only reason for changing the position is because there's
> this specific breakage. The goal should be working around that specific case
> while keeping the impact minimum on everyone else.
> It doesn't matter whether the initial change was good or bad, the kernel w/ the
> new layout is already out in the wild and it has been out there for a while. How
> is risking changing offsets on most of the fields on those kernels a good idea?
> Mimize the changes to work around the specific case.
>

Do you mean we should to update the every application
under this new order?
> > Correctly written code doesn't care.
> > Code which hardcodes layout won't notice.
>
> Huh? Code which hardcodes layout since 1.5 years ago will definitely notice.
>

As I mentioned before not all user update the kernel so frequently.
They will met this issue, if update to the 3.13,
The application failed to use which may run well previously.
> > It would be OK argument if gentlemen from Intel send "let's
> > futureproof and move Ngid because someone might depend on exact
> position" patch.
> >
> > Primum non nocere.
>
> ajlkjaeligjlakd lakjeilgjal flekjfa.
>
> --
> tejun

2015-04-21 14:00:14

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Fri, Apr 17, 2015 at 11:12:59AM -0400, Tejun Heo wrote:
> On Fri, Apr 17, 2015 at 06:05:55PM +0300, Alexey Dobriyan wrote:
> > Moving Ngid to the end of file minimizes risk of breakage.
>
> Hmmm... how so?

Correctly written parser will be unaffected:

f = fopen("/proc/self/status", "r");
while (getline(&buf, &len, f) > 0) {
if (strncmp(buf, "TracerPid:", 10) != 0)
continue;
tracer_pid = ...
}

Incorrectly written parser

buf = strchr(buf, '\n');
buf = strchr(buf, '\n');
buf = strchr(buf, '\n');
buf = strchr(buf, '\n');
buf = strchr(buf, '\n');
tracer_pid = ...

will be broken.

Moving Ngid to the end will unbreak incorrect parser and all other
incorrect parsers. There are 3 fields before Ngid and 30+ after.
What are the odds?

The only thing moving Ngid to the end is going to break is _another_
incorrect parser expecting Ngid line to be #4.

> The only reason for changing the position is because
> there's this specific breakage. The goal should be working around
> that specific case while keeping the impact minimum on everyone else.

If there are TWO incorrect parsers, one for TracerPid, another for Ngid,
you CAN'T workaround it. And if you can't workaround you choose code
which was written first, namely, TracerPid one.

In ideal world, Ngid patch will be reverted and it would be up to numa guys
to re-add it so their parsers won't break.

I briefly checked numactl code, it seems to be written correctly
(fopen+readline+strncmp), so there is hope.

> It doesn't matter whether the initial change was good or bad, the
> kernel w/ the new layout is already out in the wild and it has been
> out there for a while. How is risking changing offsets on most of the
> fields on those kernels a good idea? Mimize the changes to work
> around the specific case.
>
> > Correctly written code doesn't care.
> > Code which hardcodes layout won't notice.
>
> Huh? Code which hardcodes layout since 1.5 years ago will definitely
> notice.

2015-04-21 15:11:26

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Tue, Apr 21, 2015 at 05:00:07PM +0300, Alexey Dobriyan wrote:
> > The only reason for changing the position is because
> > there's this specific breakage. The goal should be working around
> > that specific case while keeping the impact minimum on everyone else.
>
> If there are TWO incorrect parsers, one for TracerPid, another for Ngid,
> you CAN'T workaround it. And if you can't workaround you choose code
> which was written first, namely, TracerPid one.

Not when the code has been out for 1.5 years. Minimizing the
disturbance is the better course of action. Look at the file. If you
move ngid to the end now, it's gonna shift most of the file content,
which is what caused the problem in the first place.

We don't know what's out there which again was the same problem which
triggered this thread in the first place. Why would you take the same
amount of risk when you can fix the known issue with less amount of
changes? Just put ngid after tracerpid. That way, we can fix the
known problems while changing the offsets of only four fields. At
this point, no change to the file layout is "right". Such thing isn't
defined regardless of who came first. The only thing we can do is
working around the known cases while minimizing possible impacts.

Thanks.

--
tejun

2015-04-21 15:24:05

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

"Wang, Xiaoming" <[email protected]> writes:

> Dear tejun
>
>> -----Original Message-----
>> From: [email protected] [mailto:[email protected]] On Behalf Of Tejun Heo
>> Sent: Friday, April 17, 2015 11:42 AM
>> To: Wang, Xiaoming
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; Schallberger, Timothy M;
>> Zhang, Dongxing
>> Subject: Re: [PATCH] proc: move the adding option Ngid to the end of
>> proc/PID/status
>>
>> On Thu, Apr 16, 2015 at 11:37 PM, Wang, Xiaoming
>> <[email protected]> wrote:
>> >> git describe --contains says 3.13 and it's about 1.5 years ago.
>> >>
>> > Yes this kernel change is 1.5 years ago.
>> > As we known not all user update the kernel so frequently.
>> > They just use the stable one.
>> > We met this issue when update to 3.13 now.
>> > A lot of application failed to run which run well previously.
>> > Do you have any idea on this issue?
>>
>> Not really. It's a sucky situation. How many applications are we talking about? I
>> tried to find information on libsecuritysdk but couldn't find it anywhere.
>>
> This lib libsecuritysdk is included in application.
> Taobao, weibo, tmall, alipay, etc
> It refer to security .

*cough* snake oil *cough*

Buggy non-robust code that is sold as providing a security function
deeply disturbs me. In this case libsecuritysdk is clearly buggy. The
point of labels at the beginning of lines is so that order is irrelevant.

If this had been reported by someone who cares enough to test any time
during the 6 weeks of an rc series or even shortly after a stable
release we would have take this patch immediately. Because breaking
userspace is something we don't want to do, and it would have been clear
what the trade-offs are.

In this case Tejun is right. We need to weigh the risk of fixing one
application against the risk of breaking others. So far there has been
no analysis about the possibility what other software might be affected
by this change.

With respect to testing, linux is developed as a community and it is the
responsibility for everyone in the community to pitch in and do what
they can for the bits they care about.

As best as I can infer libsecuritysdk is doing it's best to ensure that
a debugger is not attached while the library is being run. The code
appears to be binary and proprietary. So the entire community of
developers can not go out and read the code and see what is going on.
This places a higher burden on those who develop and maintian
libsecuritysdk to test and to verify their software will work with
future versions of the linux kernel, and to more promptly bring issues
to our attention.

In this instance until due dilligence has been done to indicate that
making the change proposed will not result in another bug report in
another 1.5 years from now about a different piece of software I am
inclined to strongly suggest we do nothing.

Further is there any indication that even with this small change that
the applications that use libsecuritysdk will work on 4.1-rc1 when it
comes out in the next couple of days? Or even that those applications
work on 4.0?

Eric

2015-04-23 20:32:30

[permalink] [raw]

Subject: Re: [PATCH] proc: move the adding option Ngid to the end of proc/PID/status

On Tue, Apr 21, 2015 at 11:11:19AM -0400, Tejun Heo wrote:
> On Tue, Apr 21, 2015 at 05:00:07PM +0300, Alexey Dobriyan wrote:
> > > The only reason for changing the position is because
> > > there's this specific breakage. The goal should be working around
> > > that specific case while keeping the impact minimum on everyone else.
> >
> > If there are TWO incorrect parsers, one for TracerPid, another for Ngid,
> > you CAN'T workaround it. And if you can't workaround you choose code
> > which was written first, namely, TracerPid one.
>
> Not when the code has been out for 1.5 years. Minimizing the
> disturbance is the better course of action. Look at the file. If you
> move ngid to the end now, it's gonna shift most of the file content,
> which is what caused the problem in the first place.
>
> We don't know what's out there which again was the same problem which
> triggered this thread in the first place. Why would you take the same
> amount of risk when you can fix the known issue with less amount of
> changes?

There are 2 fields before Ngid and 35+ after Ngid. So the risk is not
the same. Potentionally, Ngid addition broke almost every parser.

> Just put ngid after tracerpid. That way, we can fix the
> known problems while changing the offsets of only four fields. At
> this point, no change to the file layout is "right". Such thing isn't
> defined regardless of who came first. The only thing we can do is
> working around the known cases while minimizing possible impacts.

We'll return to this thread when next breakage will be reported,
I promise. :^)

2015-04-24 15:51:12