linux-kernel - Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fe659736-500f-4e59-a9c7-ad894155a675@moroto.mountain>
Date: Tue, 23 Jan 2024 11:56:49 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Danilo Krummrich <dakr@...hat.com>, Felix Fietkau <nbd@....name>
Cc: Colin Ian King <colin.i.king@...il.com>, nouveau@...ts.freedesktop.org,
	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable
 ret

Let's CC Felix on this one because he might know the answer.

All day long I spend looking at code like this:

net/core/dev.c:724 dev_fill_forward_path() info: returning a literal zero is cleaner
net/core/dev.c:732 dev_fill_forward_path() info: returning a literal zero is cleaner

net/core/dev.c
   696  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
   697                            struct net_device_path_stack *stack)
   698  {
   699          const struct net_device *last_dev;
   700          struct net_device_path_ctx ctx = {
   701                  .dev    = dev,
   702          };
   703          struct net_device_path *path;
   704          int ret = 0;
   705  
   706          memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
   707          stack->num_paths = 0;
   708          while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
   709                  last_dev = ctx.dev;
   710                  path = dev_fwd_path(stack);
   711                  if (!path)
   712                          return -1;
   713  
   714                  memset(path, 0, sizeof(struct net_device_path));
   715                  ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
   716                  if (ret < 0)

This if condition might trick you into thinking that ->ndo_fill_forward_path()
can return non-zero positive numbers, but it can't.  It returns zero on
success or negative error codes on failure.  Smatch is doing cross
function analysis so we know this.

   717                          return -1;
   718  
   719                  if (WARN_ON_ONCE(last_dev == ctx.dev))
   720                          return -1;
   721          }
   722  
   723          if (!ctx.dev)
   724                  return ret;

Is this intentional or not?  Who knows?  If this were an obvious bug,
I could fix it right away but ambiguous stuff like this takes way more
time to deal with.

   725  
   726          path = dev_fwd_path(stack);
   727          if (!path)
   728                  return -1;
   729          path->type = DEV_PATH_ETHERNET;
   730          path->dev = ctx.dev;
   731  
   732          return ret;

Obviously this is intentional, but if you were tricked by the checking
earlier then you might assume that ret is some positive value from the
last iteration through the loop.  "return 0;" is so much clearer.

   733  }

regards,
dan carpetner