ftrace: return error instead of 12 bytes read

A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.

Signed-off-by Roel Kluin <[email protected]>

---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/trace/trace.c +0 -0

index b20d3ec..03c7fd5 100644

Quoted 19 lines · On Wed, 11 Nov 2009 22:26:35 +0100 Roel Kluin <[email protected]> wrote:

A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.

Signed-off-by Roel Kluin <[email protected]>

---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/trace/trace.c +1 -1

index b20d3ec..03c7fd5 100644
@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 

kernel/trace/trace.c tracing_stats_read(struct file *filp, char __user *ubuf, · line 3736

Quoted 2 lines

kernel/trace/trace.c +0 -0

 	trace_seq_init(s);
 

lol, there we go again.

Andy, can we have a checkpatch rule please?

Quoted 7 lines

(quoted diff) +1 -1

@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 

Review tracing_stats_read(struct file *filp, char __user *ubuf, · line 3736

Quoted 2 lines

(quoted diff) +0 -0

 	trace_seq_init(s);
 

lol, there we go again.

Andy, can we have a checkpatch rule please?

Quoted 7 lines · Andrew Morton

Quoted 7 lines

(quoted diff) +1 -1

@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
     s = kmalloc(sizeof(*s), GFP_KERNEL);
     if (!s)
-           return ENOMEM;
+           return -ENOMEM;
 

Quoted 6 lines

Review tracing_stats_read(struct file *filp, char __user *ubuf, · line 3736

Quoted 2 lines

(quoted diff) +0 -0

     trace_seq_init(s);
 

lol, there we go again.

Andy, can we have a checkpatch rule please?

Note, that will upset creative uses of error codes i guess, such as fs/xfs/.

But yeah, +1 from me too.

Ob'post'mortem - looked for similar patterns in the kernel and there's quite a few bugs there:

 include/net/inet_hashtables.h:                  return ENOMEM;         # bug drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird fs/ocfs2/dlm/dlmrecovery.c:            return EAGAIN;                  # bug drivers/block/cciss_scsi.c:             return ENXIO;                  # works but weird drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug drivers/isdn/hardware/mISDN/hfcmulti.c:         return EINVAL;         # bug

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning would avoid real bugs here. (even ignoring the cleanliness effects of using proper error propagation)

Cc:-ed affected maintainers. The rightmost column are my observations. Below is the patch fixing these.

       Ingo

Signed-off-by Ingo Molnar <[email protected]>

Quoted 2 lines

Andy, can we have a checkpatch rule please?

Note, that will upset creative uses of error codes i guess, such as fs/xfs/.

But yeah, +1 from me too.

Ob'post'mortem - looked for similar patterns in the kernel and there's quite a few bugs there:

 include/net/inet_hashtables.h:                  return ENOMEM;         # bug drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird fs/ocfs2/dlm/dlmrecovery.c:            return EAGAIN;                  # bug drivers/block/cciss_scsi.c:             return ENXIO;                  # works but weird drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug drivers/gpu/drm/radeon/radeon_irq.c:                    return EINVAL; # bug drivers/isdn/hardware/mISDN/hfcmulti.c:         return EINVAL;         # bug

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning would avoid real bugs here. (even ignoring the cleanliness effects of using proper error propagation)

       Ingo

Quoted 6 lines · Andrew Morton

Quoted 2 lines

Andy, can we have a checkpatch rule please?

Note, that will upset creative uses of error codes i guess, such as fs/xfs/.

Quoted 8 lines

But yeah, +1 from me too.

Ob'post'mortem - looked for similar patterns in the kernel and there's quite a few bugs there:

 include/net/inet_hashtables.h:                  return ENOMEM;         # bug
 drivers/scsi/aic7xxx/aic7xxx_osm.c:             return ENOMEM;         # works but weird
 drivers/scsi/cxgb3i/cxgb3i_offload.c:           return ENOMEM;         # works but weird

drivers/scsi/aic7xxx/aic7xxx_osm.c: return ENOMEM; # works but weird

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning would avoid real bugs here. (even ignoring the cleanliness effects of using proper error propagation)

Cc:-ed affected maintainers. The rightmost column are my observations. Below is the patch fixing these.

Quoted 6 lines · Ingo Molnar

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning would avoid real bugs here. (even ignoring the cleanliness effects of using proper error propagation)

Cc:-ed affected maintainers. The rightmost column are my observations. Below is the patch fixing these.

Acked-by Joel Becker <[email protected]>

Quoted 8 lines · Joel Becker

Quoted 6 lines · Ingo Molnar

5 out of 8 places look buggy - i.e. more than 60% - a checkpatch warning would avoid real bugs here. (even ignoring the cleanliness effects of using proper error propagation)

Cc:-ed affected maintainers. The rightmost column are my observations. Below is the patch fixing these.

Quoted 1 line

Acked-by Joel Becker <[email protected]>

I take that back. NAK.

Sorry, I read the code wrong. This function is just a handler. The caller, dlm_send_begin_reco_message(), expects the positive EAGAIN as a non-error case.

Quoted 3 lines

Sorry, I read the code wrong. This function is just a handler. The caller, dlm_send_begin_reco_message(), expects the positive EAGAIN as a non-error case.

Well, at minimum the error code usage is very confused. The dlm_begin_reco_handler gets registered via o2net_register_handler(). Here's where dlm_begin_reco_handler gets registered, followed by dlm_finalize_reco_handler right afterwards:

A negative error code right there. The other event handlers are seem to be similar - dlm_begin_reco_handler() is the odd one out.

Quoted 7 lines

      s = kmalloc(sizeof(*s), GFP_KERNEL);
      if (!s)
-             return ENOMEM;
+             return -ENOMEM;

      trace_seq_init(s);

lol, there we go again.

Andy, can we have a checkpatch rule please?

Quoted 11 lines

Quoted 7 lines

? ? ? s = kmalloc(sizeof(*s), GFP_KERNEL);
? ? ? if (!s)
- ? ? ? ? ? ? return ENOMEM;
+ ? ? ? ? ? ? return -ENOMEM;

? ? ? trace_seq_init(s);

lol, there we go again.

Andy, can we have a checkpatch rule please?

Thats a tricky one. Not only do we not really have a sensible way to know if ENOMEM is an errno, we also find a bunch of places that we appear to use positive errno's as return values where we would falsly complain about. Its particularly common in scsi and filesystems. Admittedly the vast majority are return -EXXX form, so we could add this as a non-default check perhaps.

Thoughts?

So i'd suggest to make this a default-enabled check. (default disabled checks are used only by a small minority) For a _long_ time has this been the kernel standard.

Even in filesystems, ~80% of the cases use proper negative values:

 $ git grep 'return -E' fs/ | wc -l
 4540
 $ git grep 'return E' fs/ | wc -l
 895

Quoted 6 lines · Ingo Molnar

Even in filesystems, ~80% of the cases use proper negative values:

 $ git grep 'return -E' fs/ | wc -l
 4540
 $ git grep 'return E' fs/ | wc -l
 895

Except....
fs/9p/fid.c: return ERR_PTR(-EPERM);

try this:

$ git grep "return E[A-Z]*;" | grep -v EOF | grep -v ERROR | wc -l
138
$ git grep "return -E[A-Z]*;"| wc -l
57285

2 of those are in Documentation/ and 2 are comments from a quick
glance. 134 uses of positive error returns, _74_ of which are in
fs/xfs, 24 in bluetooth, and the rest scattered randomly around the kernel.

134 positive error returns vs 57881 negative? The style is so strongly ingrained in the kernel (And I'd bet an audit of those remaning 134 would find at least one bug) that it'd be a good janitor task to go through and switch them.

This is one example - a function that returns either ENXIO or 0, and the lone caller explicitly tests and flips the sign.

(Not signing off on this!!!! but CCing the relevant people for this driver)

drivers/block/cciss.c +1 -1

index 6399e50..79867d6 100644
@@ -428,7 +428,7 @@ cciss_proc_write(struct file *file, const char __user *buf,
 
                rc = cciss_engage_scsi(h->ctlr);
                if (rc != 0)
-                       err = -rc;
+                       err = rc;
                else
                        err = length;
        } else

drivers/block/cciss_scsi.c +0 -0

index 3315268..4af3085 100644

A negative error value is required: now we cannot
distinguish ENOMEM from a valid read of 12 bytes.

Signed-off-by Roel Kluin <[email protected]>

---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/trace/trace.c +1 -1

index b20d3ec..03c7fd5 100644
@@ -3730,7 +3730,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf,
 
 	s = kmalloc(sizeof(*s), GFP_KERNEL);
 	if (!s)
-		return ENOMEM;
+		return -ENOMEM;
 
 	trace_seq_init(s);
 

Ingo,

I know this is late in the rc's, but this is a very minor fix. Do you think we can slip this in before 33.

Please pull the latest tip/tracing/urgent tree, which can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/urgent

Roel Kluin (1):
tracing: Fix return value of tracing_stats_read()

----

 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)