On 10/18/21 11:12 AM, Mariusz Tkaczyk wrote:
> To avoid direct comparisions define dedicated inlines.
> This patch propagates them in super-intel.c. They are declared globally
> for future usage outside IMSM.
>
> Additionally, it adds fd check in save_backup_imsm() to remove
> code vulnerability and simplifies targets array implementation.
>
> It also propagates prr_vrb() macro instead if (verbose) condidtion.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx>
> ---
> Hi Jes,
> For now scope is limited to IMSM. If you ok the idea please apply it.
> I would like to use it in other places, but first I need to have it
> applied.
Hi Mariusz,
I am not a huge fan of hiding things in wrappers, however we've seen a
number of cases in the code with inconsistent checks for > 0 and >= 0,
so I guess it makes sense.
However I think you introduce a bug or at least a behavioral change in
the code, please see below.
> @@ -7643,26 +7635,27 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
>
> /* This device needs to be a device in an 'imsm' container */
> fd = open(dev, O_RDONLY|O_EXCL, 0);
> - if (fd >= 0) {
> - if (verbose)
> - pr_err("Cannot create this array on device %s\n",
> - dev);
> +
> + if (is_fd_valid(fd)) {
> + pr_vrb("Cannot create this array on device %s\n", dev);
> close(fd);
> return 0;
> }
> - if (errno != EBUSY || (fd = open(dev, O_RDONLY, 0)) < 0) {
> - if (verbose)
> - pr_err("Cannot open %s: %s\n",
> - dev, strerror(errno));
> - return 0;
> + if (errno != EBUSY) {
> + fd = open(dev, O_RDONLY, 0);
> +
> + if (!is_fd_valid(fd)) {
> + pr_vrb("Cannot open %s: %s\n", dev, strerror(errno));
> + return 0;
> + }
> }
The old code would only call open(dev, O_RDONLY, 0) if errno == EBUSY,
and it enters the if() block unconditionally, unless errno == EBUSY and
we fail to open O_RDONLY.
With your change you try to reopen the device for all error cases,
except for EBUSY.
I assume this is a mistake? If you do want to change the logic, it would
be preferred to do it in a separate patch.
Cheers,
Jes
