linux-kernel - Re: [PATCH] mtd: phram: error handling

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151110190343.GR12143@google.com>
Date:	Tue, 10 Nov 2015 11:03:43 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Saurabh Sengar <saurabh.truth@...il.com>,
	andy.shevchenko@...il.com, joern@...ybastard.org,
	dwmw2@...radead.org, linux-mtd@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: phram: error handling

On Tue, Nov 10, 2015 at 10:45:55AM -0800, Joe Perches wrote:
> On Tue, 2015-11-10 at 10:39 -0800, Brian Norris wrote:
> > On Tue, Nov 10, 2015 at 10:33:07AM -0800, Joe Perches wrote:
> > > Expand parse_err macro with hidden flow in-place.
> > > Remove the now unused parse_err macro.
> []
> > I think -EINVAL makes more sense than 1. That
> > could be a subsequent patch, I suppose.
> 
> That means you have to trace all the callers
> to verify that converting 1 to -22 is acceptable.

It's fairly simple. Module initialization and module parameter calls
both *should* follow 0/negative error conventions. For module init, see
in kernel/module.c:

        /* Start the module */
        if (mod->init != NULL)
                ret = do_one_initcall(mod->init);
        if (ret < 0) {
                goto fail_free_freeinit;
        }
        if (ret > 0) {
                pr_warn("%s: '%s'->init suspiciously returned %d, it should "
                        "follow 0/-E convention\n"
                        "%s: loading module anyway...\n",
                        __func__, mod->name, ret, __func__);
                dump_stack();
        }

and in include/linux/moduleparam.h:

struct kernel_param_ops {
...
        /* Returns 0, or -errno.  arg is in kp->arg. */
        int (*set)(const char *val, const struct kernel_param *kp);
...
};

And for built-in modules, the return code is ignored (see
do_initcall_level()).

So I think the only question is whether we should actually be reporting
these errors on module insertion and on the module parameter call. I'd
say "definitely" to the latter and "yes" to the former, since the init
function already handles the case of an empty input (so the module can
be loaded with a blank command line without tripping on an -EINVAL
parameter check).

IOW, to use -EINVAL would be to actually enforce the error handling that
was intended in the first place. But that should be done in a separate
patch, and with an actual tester (since I doubt either you or Saurabh
are testing this driver).

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