[LU-6519] potential null pointer dereference in class_newdev

• Type: Bug
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.11.0
• Labels: smatch
• Severity: 3
• Assignee: WC Triage
• Reporter: Oleg Drokin

Description

smatch highlighted this in class_newdev:

        if (result == NULL && i >= class_devno_max()) {
                CERROR("all %u OBD devices used, increase MAX_OBD_DEVICES\n",
                       class_devno_max());
                GOTO(out, result = ERR_PTR(-EOVERFLOW));
        }

        if (IS_ERR(result))
                GOTO(out, result);

        CDEBUG(D_IOCTL, "Adding new device %s (%p)\n",
               result->obd_name, result);

So this totally seems to assume that if result == NULL, but we did not go beyond the obd device limit, we are ok to print this NULL pointer?

I suspect we should chance the IS_ERRO to either result == NULL || IS_ERR?

Issue Links

• is related to: LU-4134 obdfilter-suvery bugs and panics (ioctl API isn't protected over shutdown/setup property).

Comments

[Andreas Dilger]

Fixed in patch https://review.whamcloud.com/8045 ("LU-4134 obdclass: obd_device improvement")

[Ulka Vaze (Inactive)]

Hi,
Following is analysis from my side -
Below is relevent part of code of newdev

 for (i = 0; i < class_devno_max(); i++) {
                struct obd_device *obd = class_num2obd(i);

        if (obd && (strcmp(name, obd->obd_name) == 0)) {
                        CERROR("Device %s already exists at %d, won't add\n",
                               name, i);
                        if (result) {
                                LASSERTF(result->obd_magic == OBD_DEVICE_MAGIC,
                                         "%p obd_magic %08x != %08x\n", result,
                                         result->obd_magic, OBD_DEVICE_MAGIC);
                                LASSERTF(result->obd_minor == new_obd_minor,
                                         "%p obd_minor %d != %d\n", result,
                                         result->obd_minor, new_obd_minor);

                                obd_devs[result->obd_minor] = NULL;
                                result->obd_name[0]='\0';
                         }
                        result = ERR_PTR(-EEXIST);
                        break;
                }
                if (!result && !obd) {
                        result = newdev;
                        result->obd_minor = i;
                        new_obd_minor = i;
                        result->obd_type = type;
                        strncpy(result->obd_name, name,
                                sizeof(result->obd_name) - 1);
        obd_devs[i] = result;
                }
        }
    write_unlock(&obd_dev_lock);

        if (result == NULL && i >= class_devno_max()) {
                CERROR("all %u OBD devices used, increase MAX_OBD_DEVICES\n",
                       class_devno_max());
        GOTO(out, result = ERR_PTR(-EOVERFLOW));
        }

1. if result is NULL and loop is completed this means no slot is found for new obd. This case is handled ihere -

if (result == NULL && i >= class_devno_max())

{ CERROR("all %u OBD devices used, increase MAX_OBD_DEVICES\n", class_devno_max()); GOTO(out, result = ERR_PTR(-EOVERFLOW)); }

and result value is reassigned. We printing max no of obd devices.
2. If we break out of loop because obd exists then result will have error code -EEXIST so result will no be NULL
Out code frees the menory allocated to newdev.
This code seems fine to me and probabally a false alarm from the tool.
Please confirm.

-Ulka