[PATCH 0/12] Further PDE->data assignments cleanups - Kernel

This is a discussion on [PATCH 0/12] Further PDE->data assignments cleanups - Kernel ; From: "Denis V. Lunev" Date: Tue, 29 Apr 2008 15:13:19 +0400 > The check for PDE->data != NULL becomes useless after the replacement > of proc_net_fops_create with proc_create_data. > > Signed-off-by: Denis V. Lunev Applied. -- To unsubscribe from this ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 27 of 27

Thread: [PATCH 0/12] Further PDE->data assignments cleanups

  1. Re: [PATCH 9/12] ipv4: assign PDE->data before gluing PDE into /proc tree

    From: "Denis V. Lunev"
    Date: Tue, 29 Apr 2008 15:13:19 +0400

    > The check for PDE->data != NULL becomes useless after the replacement
    > of proc_net_fops_create with proc_create_data.
    >
    > Signed-off-by: Denis V. Lunev


    Applied.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 12/12] netns: assign PDE->data before gluing entry into /proc tree

    From: "Denis V. Lunev"
    Date: Tue, 29 Apr 2008 15:13:22 +0400

    > In this unfortunate case, proc_mkdir_mode wrapper can't be used anymore and
    > this is no way to reuse proc_create_data due to nlinks assignment. So,
    > copy the code from proc_mkdir and assign PDE->data at the appropriate
    > moment.
    >
    > Signed-off-by: Denis V. Lunev


    Oh well, the cost of correctness sometimes :-)

    Applied, thanks.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 11/12] netfilter: assign PDE->data before gluing PDE into /proc tree

    From: "Denis V. Lunev"
    Date: Tue, 29 Apr 2008 15:13:21 +0400

    > Replace proc_net_fops_create with proc_create_data.
    >
    > Signed-off-by: Denis V. Lunev
    > Cc: Alexey Dobriyan
    > Cc: Eric W. Biederman
    > Cc: Patrick McHardy


    Applied, with Patrick's ACK.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 10/12] netfilter: assign PDE->fops before gluing PDE into /proc tree

    From: "Denis V. Lunev"
    Date: Tue, 29 Apr 2008 15:13:20 +0400

    > Replace create_proc_entry with specially created for this purpose proc_create.
    >
    > Signed-off-by: Denis V. Lunev
    > Cc: Alexey Dobriyan
    > Cc: Eric W. Biederman
    > Cc: Patrick McHardy


    Applied, with Patrick's ACK.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 9/12] ipv4: assign PDE->data before gluing PDE into /proc tree

    First off, sorry to bring such an old email back but I can seem to get
    a bad feeling when looking back over it.

    On Tue, Apr 29, 2008 at 6:13 AM, Denis V. Lunev wrote:
    > The check for PDE->data != NULL becomes useless after the replacement
    > of proc_net_fops_create with proc_create_data.
    >
    > Signed-off-by: Denis V. Lunev
    > Cc: Alexey Dobriyan
    > Cc: Eric W. Biederman
    > Cc: David S. Miller
    > ---
    > net/ipv4/tcp_ipv4.c | 10 +++-------
    > net/ipv4/udp.c | 7 +++----
    > 2 files changed, 6 insertions(+), 11 deletions(-)
    >
    > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
    > index 7766151..4d97b28 100644
    > --- a/net/ipv4/tcp_ipv4.c
    > +++ b/net/ipv4/tcp_ipv4.c
    > @@ -2214,9 +2214,6 @@ static int tcp_seq_open(struct inode *inode, struct file *file)
    > struct tcp_iter_state *s;
    > int err;
    >
    > - if (unlikely(afinfo == NULL))
    > - return -EINVAL;

    I think that this check needs to stay in some form, reason below.
    > -
    > err = seq_open_net(inode, file, &afinfo->seq_ops,
    > sizeof(struct tcp_iter_state));
    > if (err < 0)
    > @@ -2241,10 +2238,9 @@ int tcp_proc_register(struct net *net, struct tcp_seq_afinfo *afinfo)
    > afinfo->seq_ops.next = tcp_seq_next;
    > afinfo->seq_ops.stop = tcp_seq_stop;
    >
    > - p = proc_net_fops_create(net, afinfo->name, S_IRUGO, &afinfo->seq_fops);
    > - if (p)
    > - p->data = afinfo;
    > - else
    > + p = proc_create_data(afinfo->name, S_IRUGO, net->proc_net,


    When you try to pass in afinfo->name (and also the seq_fops) you are
    assuming that afinfo is not null meaning in the unlikely(as shown
    above) even that it is null you get a very bad null pointer problem.
    If I am just way off do let me know because this just seams to me like
    a bad idea. This is also still present in 2.6.26-rc9.

    -Stoyan G

    > + &afinfo->seq_fops, afinfo);
    > + if (!p)
    > rc = -ENOMEM;
    > return rc;
    > }
    > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
    > index b053ac7..c19c491 100644
    > --- a/net/ipv4/udp.c
    > +++ b/net/ipv4/udp.c
    > @@ -1605,10 +1605,9 @@ int udp_proc_register(struct net *net, struct udp_seq_afinfo *afinfo)
    > afinfo->seq_ops.next = udp_seq_next;
    > afinfo->seq_ops.stop = udp_seq_stop;
    >
    > - p = proc_net_fops_create(net, afinfo->name, S_IRUGO, &afinfo->seq_fops);
    > - if (p)
    > - p->data = afinfo;
    > - else
    > + p = proc_create_data(afinfo->name, S_IRUGO, net->proc_net,
    > + &afinfo->seq_fops, afinfo);
    > + if (!p)
    > rc = -ENOMEM;
    > return rc;
    > }
    > --
    > 1.5.3.rc5
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 9/12] ipv4: assign PDE->data before gluing PDE into /proc tree

    "Stoyan Gaydarov" writes:

    > First off, sorry to bring such an old email back but I can seem to get
    > a bad feeling when looking back over it.
    >
    > On Tue, Apr 29, 2008 at 6:13 AM, Denis V. Lunev wrote:
    >> The check for PDE->data != NULL becomes useless after the replacement
    >> of proc_net_fops_create with proc_create_data.
    >>
    >> Signed-off-by: Denis V. Lunev
    >> Cc: Alexey Dobriyan
    >> Cc: Eric W. Biederman
    >> Cc: David S. Miller
    >> ---
    >> net/ipv4/tcp_ipv4.c | 10 +++-------
    >> net/ipv4/udp.c | 7 +++----
    >> 2 files changed, 6 insertions(+), 11 deletions(-)
    >>
    >> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
    >> index 7766151..4d97b28 100644
    >> --- a/net/ipv4/tcp_ipv4.c
    >> +++ b/net/ipv4/tcp_ipv4.c
    >> @@ -2214,9 +2214,6 @@ static int tcp_seq_open(struct inode *inode, struct file

    > *file)
    >> struct tcp_iter_state *s;
    >> int err;
    >>
    >> - if (unlikely(afinfo == NULL))
    >> - return -EINVAL;

    > I think that this check needs to stay in some form, reason below.
    >> -
    >> err = seq_open_net(inode, file, &afinfo->seq_ops,
    >> sizeof(struct tcp_iter_state));
    >> if (err < 0)
    >> @@ -2241,10 +2238,9 @@ int tcp_proc_register(struct net *net, struct

    > tcp_seq_afinfo *afinfo)
    >> afinfo->seq_ops.next = tcp_seq_next;
    >> afinfo->seq_ops.stop = tcp_seq_stop;
    >>
    >> - p = proc_net_fops_create(net, afinfo->name, S_IRUGO, &afinfo->seq_fops);
    >> - if (p)
    >> - p->data = afinfo;
    >> - else
    >> + p = proc_create_data(afinfo->name, S_IRUGO, net->proc_net,

    >
    > When you try to pass in afinfo->name (and also the seq_fops) you are
    > assuming that afinfo is not null meaning in the unlikely(as shown
    > above) even that it is null you get a very bad null pointer problem.
    > If I am just way off do let me know because this just seams to me like
    > a bad idea. This is also still present in 2.6.26-rc9.


    It appears you are getting things confused. The original window is that tcp_seq_open
    (which is what get called when you open the proc file) had a small race that p->data
    could be read before it was set.

    With proc_create_data that race was closed.

    You are saying that it is a problem for tcp_seq_open to be passed a NULL afinfo.
    It is. That has nothing to do with the original race (as that is a very
    different part of the code). Feel free to audit all of the callers if
    you like. That problem however is not subtle or racy.

    So I see nothing wrong with this patch unless you can find a problem with
    proc_create_data.

    Eric
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 9/12] ipv4: assign PDE->data before gluing PDE into /proc tree

    On Fri, 2008-07-11 at 22:12 -0500, Stoyan Gaydarov wrote:
    > First off, sorry to bring such an old email back but I can seem to get
    > a bad feeling when looking back over it.
    >
    > On Tue, Apr 29, 2008 at 6:13 AM, Denis V. Lunev wrote:
    > > The check for PDE->data != NULL becomes useless after the replacement
    > > of proc_net_fops_create with proc_create_data.
    > >
    > > Signed-off-by: Denis V. Lunev
    > > Cc: Alexey Dobriyan
    > > Cc: Eric W. Biederman
    > > Cc: David S. Miller
    > > ---
    > > net/ipv4/tcp_ipv4.c | 10 +++-------
    > > net/ipv4/udp.c | 7 +++----
    > > 2 files changed, 6 insertions(+), 11 deletions(-)
    > >
    > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
    > > index 7766151..4d97b28 100644
    > > --- a/net/ipv4/tcp_ipv4.c
    > > +++ b/net/ipv4/tcp_ipv4.c
    > > @@ -2214,9 +2214,6 @@ static int tcp_seq_open(struct inode *inode, struct file *file)
    > > struct tcp_iter_state *s;
    > > int err;
    > >
    > > - if (unlikely(afinfo == NULL))
    > > - return -EINVAL;

    > I think that this check needs to stay in some form, reason below.
    > > -
    > > err = seq_open_net(inode, file, &afinfo->seq_ops,
    > > sizeof(struct tcp_iter_state));
    > > if (err < 0)
    > > @@ -2241,10 +2238,9 @@ int tcp_proc_register(struct net *net, struct tcp_seq_afinfo *afinfo)
    > > afinfo->seq_ops.next = tcp_seq_next;
    > > afinfo->seq_ops.stop = tcp_seq_stop;
    > >
    > > - p = proc_net_fops_create(net, afinfo->name, S_IRUGO, &afinfo->seq_fops);
    > > - if (p)
    > > - p->data = afinfo;
    > > - else
    > > + p = proc_create_data(afinfo->name, S_IRUGO, net->proc_net,

    >
    > When you try to pass in afinfo->name (and also the seq_fops) you are
    > assuming that afinfo is not null meaning in the unlikely(as shown
    > above) even that it is null you get a very bad null pointer problem.
    > If I am just way off do let me know because this just seams to me like
    > a bad idea. This is also still present in 2.6.26-rc9.


    The reason to remove the check is simple - afinfo comes in the form of
    the static pointer during init time. It is impossible to face NULL
    pointer the problem in the reality. It can come as the programmer
    mistake, but the OOPS will be immediate and straightforward. Namely, the
    kernel will not boot/work at all.

    Regards,
    Den

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2