possible kernel bug in signal transit.

possible kernel bug in signal transit.

[Alex Lyashkov]

Hello All

I analyze kernel vanila 2.6.4 and found one possible bug in
__kill_pg_info function.

        for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
                err = group_send_sig_info(sig, info, p);
                if (retval)
                        retval = err;
        }
but I think if (retval) is incorrect check. possible this cycle must be
        for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
                err = group_send_sig_info(sig, info, p);
                if (ret) {
                        retval = err;
			break;
		}
        }
because in original variant me assign to retval only first value from
ret and other be ignored if this value be 0.


-- 
Alex Lyashkov <shadow@psoft.net>
PSoft

Re: possible kernel bug in signal transit.

[Andrew Morton]

Alex Lyashkov <shadow@psoft.net> wrote:
>
> Hello All
> 
> I analyze kernel vanila 2.6.4 and found one possible bug in
> __kill_pg_info function.
> 
>         for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
>                 err = group_send_sig_info(sig, info, p);
>                 if (retval)
>                         retval = err;
>         }
> but I think if (retval) is incorrect check. possible this cycle must be
>         for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
>                 err = group_send_sig_info(sig, info, p);
>                 if (ret) {
>                         retval = err;
> 			break;
> 		}
>         }
> because in original variant me assign to retval only first value from
> ret and other be ignored if this value be 0.
> 

No, the code's OK, albeit undesirably obscure.  It will return -ESRCH if
none of the tasks had a matching pgrp and will return the result of the
final non-zero-returning group_send_sig_info() if one or more of the
group_send_sig_info() calls failed, and will return zero if all of the
group_send_sig_info() calls returned zero.

Thanks for checking though..

Re: possible kernel bug in signal transit.

[Alex Lyashkov]

В Вск, 14.03.2004, в 03:18, Andrew Morton пишет:
> Alex Lyashkov <shadow@psoft.net> wrote:
> >
> > Hello All
> > 
> > I analyze kernel vanila 2.6.4 and found one possible bug in
> > __kill_pg_info function.
> > 
> >         for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> >                 err = group_send_sig_info(sig, info, p);
> >                 if (retval)
> >                         retval = err;
> >         }
> > but I think if (retval) is incorrect check. possible this cycle must be
> >         for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> >                 err = group_send_sig_info(sig, info, p);
> >                 if (ret) {
> >                         retval = err;
> > 			break;
> > 		}
> >         }
> > because in original variant me assign to retval only first value from
> > ret and other be ignored if this value be 0.
> > 
> 
> No, the code's OK, albeit undesirably obscure.  It will return -ESRCH if
> none of the tasks had a matching pgrp and will return the result of the
> final non-zero-returning group_send_sig_info() if one or more of the
> group_send_sig_info() calls failed, and will return zero if all of the
> group_send_sig_info() calls returned zero.
> 
> Thanks for checking though..
No. it can`t return final non-zero-returning group_send_sig_info() if
first call group_send_sig_info return 0.
Example me have 3 groups it will return
1 - zero
2 - nonzero (example -1)
3 - nonzero (example -2)
original code will return zero.
but you say it will return last non zero - "-2" in example.
(do only first assignment after it retval is zero and no assignments
does.)
For her logic me can write.
=====
	int retval = 0;
	int err = -1;
        for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
                 err = group_send_sig_info(sig, info, p);
		if( err )
			retval = err;

         }
	return err==-1 ? -ESRCH : retval;
===
If me not enter to this cycle - retval = -ESRCH, and last non zero err
if cycle is worked or zero if all group_send_sig_info() return zero.


-- 
Alex Lyashkov <shadow@psoft.net>
PSoft

Re: possible kernel bug in signal transit.

[Andrew Morton]

Alex Lyashkov <shadow@psoft.net> wrote:
>
> > Thanks for checking though..
>  No. it can`t return final non-zero-returning group_send_sig_info() if
>  first call group_send_sig_info return 0.

you're right.   How about the nice and simple version?

int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
{
	struct task_struct *p;
	struct list_head *l;
	struct pid *pid;
	int retval;
	int found;

	if (pgrp <= 0)
		return -EINVAL;

	found = 0;
	retval = 0;
	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
		int err;

		found = 1;
		err = group_send_sig_info(sig, info, p);
		if (!retval)
			retval = err;
	}
	return found ? retval : -ESRCH;
}

Re: possible kernel bug in signal transit.

[Alex Lyashkov]

В Вск, 14.03.2004, в 07:00, Andrew Morton пишет:
> Alex Lyashkov <shadow@psoft.net> wrote:
> >
> > > Thanks for checking though..
> >  No. it can`t return final non-zero-returning group_send_sig_info() if
> >  first call group_send_sig_info return 0.
> 
> you're right.   How about the nice and simple version?
> 
> int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
> {
> 	struct task_struct *p;
> 	struct list_head *l;
> 	struct pid *pid;
> 	int retval;
> 	int found;
> 
> 	if (pgrp <= 0)
> 		return -EINVAL;
> 
> 	found = 0;
> 	retval = 0;
> 	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> 		int err;
> 
> 		found = 1;
> 		err = group_send_sig_info(sig, info, p);
> 		if (!retval)
> 			retval = err;
> 	}
> 	return found ? retval : -ESRCH;
> }
not. it error. At this code you save first non zero value err but other
been ignored.


-- 
Alex Lyashkov <shadow@psoft.net>
PSoft

Re: possible kernel bug in signal transit.

[Andrew Morton]

Alex Lyashkov <shadow@psoft.net> wrote:
>
> > int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
>  > {
>  > 	struct task_struct *p;
>  > 	struct list_head *l;
>  > 	struct pid *pid;
>  > 	int retval;
>  > 	int found;
>  > 
>  > 	if (pgrp <= 0)
>  > 		return -EINVAL;
>  > 
>  > 	found = 0;
>  > 	retval = 0;
>  > 	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
>  > 		int err;
>  > 
>  > 		found = 1;
>  > 		err = group_send_sig_info(sig, info, p);
>  > 		if (!retval)
>  > 			retval = err;
>  > 	}
>  > 	return found ? retval : -ESRCH;
>  > }
>  not. it error. At this code you save first non zero value err but other
>  been ignored.

Well we can only return one error code.  Or are you suggesting that we
should terminate the loop early on error?  If so, why?

Re: possible kernel bug in signal transit.

[Alex Lyashkov]

В Вск, 14.03.2004, в 07:47, Andrew Morton пишет:
> Alex Lyashkov <shadow@psoft.net> wrote:
> >
> > > int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
> >  > {
> >  > 	struct task_struct *p;
> >  > 	struct list_head *l;
> >  > 	struct pid *pid;
> >  > 	int retval;
> >  > 	int found;
> >  > 
> >  > 	if (pgrp <= 0)
> >  > 		return -EINVAL;
> >  > 
> >  > 	found = 0;
> >  > 	retval = 0;
> >  > 	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> >  > 		int err;
> >  > 
> >  > 		found = 1;
> >  > 		err = group_send_sig_info(sig, info, p);
> >  > 		if (!retval)
> >  > 			retval = err;
> >  > 	}
> >  > 	return found ? retval : -ESRCH;
> >  > }
> >  not. it error. At this code you save first non zero value err but other
> >  been ignored.
> 
> Well we can only return one error code.  Or are you suggesting that we
> should terminate the loop early on error?  If so, why?
You say me can return _last_ error core. but this function return
_first_. 

I write second variant where not terminate loop and save _last_ error
code (i was sending in previous mail). but if you have i write full
function:
====
int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp)
{
	struct task_struct *p;
	struct list_head *l;
	struct pid *pid;
 	int retval = 0;
 	int err = -1;
 
 	if (pgrp <= 0)
 		return -EINVAL;

        for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
                 err = group_send_sig_info(sig, info, p);
                if( err )
                        retval = err;

         }
        return err==-1 ? -ESRCH : retval;
}
===
what you think about its code ?


-- 
Alex Lyashkov <shadow@psoft.net>
PSoft

Re: possible kernel bug in signal transit.

[Andrew Morton]

Alex Lyashkov <shadow@psoft.net> wrote:
>
> > Well we can only return one error code.  Or are you suggesting that we
>  > should terminate the loop early on error?  If so, why?
>  You say me can return _last_ error core. but this function return
>  _first_. 

It doesn't matter, really.  Other parts of the kernel will generally return
the first-encountered error because at times it _does_ matter.  But here it
doesn't.

Re: possible kernel bug in signal transit.

[Alex Lyashkov]

В Вск, 14.03.2004, в 08:09, Andrew Morton пишет:
> Alex Lyashkov <shadow@psoft.net> wrote:
> >
> > > Well we can only return one error code.  Or are you suggesting that we
> >  > should terminate the loop early on error?  If so, why?
> >  You say me can return _last_ error core. but this function return
> >  _first_. 
> 
> It doesn't matter, really.  Other parts of the kernel will generally return
> the first-encountered error because at times it _does_ matter.  But here it
> doesn't.
okey. second question.
a really need extra variable instead change conditions in return ?


-- 
Alex Lyashkov <shadow@psoft.net>
PSoft

Re: possible kernel bug in signal transit.

[Andrew Morton]

Alex Lyashkov <shadow@psoft.net> wrote:
>
> __ ______, 14.03.2004, __ 08:09, Andrew Morton __________:
> > Alex Lyashkov <shadow@psoft.net> wrote:
> > >
> > > > Well we can only return one error code.  Or are you suggesting that we
> > >  > should terminate the loop early on error?  If so, why?
> > >  You say me can return _last_ error core. but this function return
> > >  _first_. 
> > 
> > It doesn't matter, really.  Other parts of the kernel will generally return
> > the first-encountered error because at times it _does_ matter.  But here it
> > doesn't.
> okey. second question.
> a really need extra variable instead change conditions in return ?

I think it's clearer that way, don't you?