| Message ID | 20210816014456.2191776-2-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libata cleanups and improvements | expand |
On 8/16/21 3:44 AM, Damien Le Moal wrote: > Avoid static checkers warnings about a potential NULL pointer > dereference for the port info variable pi. To do so, test that at least > one port info is available on entry to ata_host_alloc_pinfo() and start > the ata port initialization for() loop with pi initialized to the first > port info passed as argument (which is already checked to be non NULL). > Within the for() loop, get the next port info, if it is not NULL, > after initializing the ata port using the previous port info. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/ata/libata-core.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Mon, 2021-08-16 at 10:44 +0900, Damien Le Moal wrote: > Avoid static checkers warnings about a potential NULL pointer > dereference for the port info variable pi. To do so, test that at > least > one port info is available on entry to ata_host_alloc_pinfo() and > start > the ata port initialization for() loop with pi initialized to the > first > port info passed as argument (which is already checked to be non > NULL). > Within the for() loop, get the next port info, if it is not NULL, > after initializing the ata port using the previous port info. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/ata/libata-core.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 61c762961ca8..b237a718ea0f 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5441,16 +5441,17 @@ struct ata_host *ata_host_alloc_pinfo(struct > device *dev, > struct ata_host *host; > int i, j; > > + /* We must have at least one port info */ > + if (!ppi[0]) > + return NULL; I've got to ask why on this one: most libata drivers use a static array for the port info. If the first element is NULL that's a coding failure inside the driver, so WARN_ON would probably be more helpful to the driver writer. What makes the static checker think ppi isn't NULL? > + > host = ata_host_alloc(dev, n_ports); > if (!host) > return NULL; > > - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { > + for (i = 0, j = 0, pi = ppi[0]; i < host->n_ports; i++) { > struct ata_port *ap = host->ports[i]; > > - if (ppi[j]) > - pi = ppi[j++]; > - > ap->pio_mask = pi->pio_mask; > ap->mwdma_mask = pi->mwdma_mask; > ap->udma_mask = pi->udma_mask; > @@ -5460,6 +5461,15 @@ struct ata_host *ata_host_alloc_pinfo(struct > device *dev, > > if (!host->ops && (pi->port_ops != > &ata_dummy_port_ops)) > host->ops = pi->port_ops; > + > + /* > + * Check that the next port info is not NULL. > + * If it is, keep using the current one. > + */ > + if (j < n_ports - 1 && ppi[j + 1]) { > + j++; > + pi = ppi[j]; > + } This looks completely pointless: once you've verified ppi[0] is not NULL above, there's no possible NULL deref in that loop and the static checker should see it. If it doesn't we need a new static checker because we shouldn't be perturbing code for broken tools. James
On 2021/08/16 20:29, James Bottomley wrote: > On Mon, 2021-08-16 at 10:44 +0900, Damien Le Moal wrote: >> Avoid static checkers warnings about a potential NULL pointer >> dereference for the port info variable pi. To do so, test that at >> least >> one port info is available on entry to ata_host_alloc_pinfo() and >> start >> the ata port initialization for() loop with pi initialized to the >> first >> port info passed as argument (which is already checked to be non >> NULL). >> Within the for() loop, get the next port info, if it is not NULL, >> after initializing the ata port using the previous port info. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/ata/libata-core.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index 61c762961ca8..b237a718ea0f 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -5441,16 +5441,17 @@ struct ata_host *ata_host_alloc_pinfo(struct >> device *dev, >> struct ata_host *host; >> int i, j; >> >> + /* We must have at least one port info */ >> + if (!ppi[0]) >> + return NULL; > > I've got to ask why on this one: most libata drivers use a static array > for the port info. If the first element is NULL that's a coding > failure inside the driver, so WARN_ON would probably be more helpful to > the driver writer. > > What makes the static checker think ppi isn't NULL? > >> + >> host = ata_host_alloc(dev, n_ports); >> if (!host) >> return NULL; >> >> - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { >> + for (i = 0, j = 0, pi = ppi[0]; i < host->n_ports; i++) { >> struct ata_port *ap = host->ports[i]; >> >> - if (ppi[j]) >> - pi = ppi[j++]; >> - >> ap->pio_mask = pi->pio_mask; >> ap->mwdma_mask = pi->mwdma_mask; >> ap->udma_mask = pi->udma_mask; >> @@ -5460,6 +5461,15 @@ struct ata_host *ata_host_alloc_pinfo(struct >> device *dev, >> >> if (!host->ops && (pi->port_ops != >> &ata_dummy_port_ops)) >> host->ops = pi->port_ops; >> + >> + /* >> + * Check that the next port info is not NULL. >> + * If it is, keep using the current one. >> + */ >> + if (j < n_ports - 1 && ppi[j + 1]) { >> + j++; >> + pi = ppi[j]; >> + } > > This looks completely pointless: once you've verified ppi[0] is not > NULL above, there's no possible NULL deref in that loop and the static > checker should see it. If it doesn't we need a new static checker > because we shouldn't be perturbing code for broken tools. I do not know how to run that static checker which sent the warnings initially. I changed the code to avoid all the "dumb" cases it thinks are possible and leading to the NULL deref signaled. I think we should drop this patch. If the checker complains again, then I can revisit in a different series. Jens, can you drop this one when you apply the series (if you think it is good to apply). Or should I resend a v8 without this patch ? > > James > > >
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 61c762961ca8..b237a718ea0f 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5441,16 +5441,17 @@ struct ata_host *ata_host_alloc_pinfo(struct device *dev, struct ata_host *host; int i, j; + /* We must have at least one port info */ + if (!ppi[0]) + return NULL; + host = ata_host_alloc(dev, n_ports); if (!host) return NULL; - for (i = 0, j = 0, pi = NULL; i < host->n_ports; i++) { + for (i = 0, j = 0, pi = ppi[0]; i < host->n_ports; i++) { struct ata_port *ap = host->ports[i]; - if (ppi[j]) - pi = ppi[j++]; - ap->pio_mask = pi->pio_mask; ap->mwdma_mask = pi->mwdma_mask; ap->udma_mask = pi->udma_mask; @@ -5460,6 +5461,15 @@ struct ata_host *ata_host_alloc_pinfo(struct device *dev, if (!host->ops && (pi->port_ops != &ata_dummy_port_ops)) host->ops = pi->port_ops; + + /* + * Check that the next port info is not NULL. + * If it is, keep using the current one. + */ + if (j < n_ports - 1 && ppi[j + 1]) { + j++; + pi = ppi[j]; + } } return host;
Avoid static checkers warnings about a potential NULL pointer dereference for the port info variable pi. To do so, test that at least one port info is available on entry to ata_host_alloc_pinfo() and start the ata port initialization for() loop with pi initialized to the first port info passed as argument (which is already checked to be non NULL). Within the for() loop, get the next port info, if it is not NULL, after initializing the ata port using the previous port info. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/ata/libata-core.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)