* [bug report] net: ocelot: Extend MRP
@ 2021-03-18 12:07 Dan Carpenter
2021-03-18 12:11 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-03-18 12:07 UTC (permalink / raw)
To: horatiu.vultur; +Cc: kernel-janitors
Hello Horatiu Vultur,
This is a semi-automatic email about new static checker warnings.
The patch 7c588c3e96e9: "net: ocelot: Extend MRP" from Mar 16, 2021,
leads to the following Smatch complaint:
drivers/net/ethernet/mscc/ocelot_mrp.c:180 ocelot_mrp_del()
error: we previously assumed 'ocelot_port' could be null (see line 173)
drivers/net/ethernet/mscc/ocelot_mrp.c
153 int ocelot_mrp_del(struct ocelot *ocelot, int port,
154 const struct switchdev_obj_mrp *mrp)
155 {
156 struct ocelot_port *ocelot_port = ocelot->ports[port];
157 int i;
158
159 if (!ocelot_port)
160 return -EOPNOTSUPP;
161
162 if (ocelot_port->mrp_ring_id != mrp->ring_id)
163 return 0;
164
165 ocelot_mrp_del_vcap(ocelot, port);
166 ocelot_mrp_del_vcap(ocelot, port + ocelot->num_phys_ports);
167
168 ocelot_port->mrp_ring_id = 0;
169
170 for (i = 0; i < ocelot->num_phys_ports; ++i) {
This loop tries to verify that all the ports have ring_id == 0 otherwise
we return. It's slightly confusing why this matters.
171 ocelot_port = ocelot->ports[i];
172
173 if (!ocelot_port)
^^^^^^^^^^^
Assume the last element of the array is NULL
174 continue;
175
176 if (ocelot_port->mrp_ring_id != 0)
177 goto out;
This would be more clear if instead of a "goto out;" it just did a
direct "return 0;"
178 }
179
180 ocelot_mrp_del_mac(ocelot, ocelot_port);
We delete the last mac address of all the ring_ids are zero but if the
last port is zero it will crash.
181 out:
182 return 0;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net: ocelot: Extend MRP
2021-03-18 12:07 [bug report] net: ocelot: Extend MRP Dan Carpenter
@ 2021-03-18 12:11 ` Dan Carpenter
2021-03-18 18:57 ` Horatiu Vultur
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-03-18 12:11 UTC (permalink / raw)
To: horatiu.vultur; +Cc: kernel-janitors
See also:
drivers/net/ethernet/mscc/ocelot_mrp.c:254 ocelot_mrp_del_ring_role() error: we previously assumed 'ocelot_port' could be null (see line 247)
regards,
dan carpenter
On Thu, Mar 18, 2021 at 03:07:32PM +0300, Dan Carpenter wrote:
> Hello Horatiu Vultur,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 7c588c3e96e9: "net: ocelot: Extend MRP" from Mar 16, 2021,
> leads to the following Smatch complaint:
>
> drivers/net/ethernet/mscc/ocelot_mrp.c:180 ocelot_mrp_del()
> error: we previously assumed 'ocelot_port' could be null (see line 173)
>
> drivers/net/ethernet/mscc/ocelot_mrp.c
> 153 int ocelot_mrp_del(struct ocelot *ocelot, int port,
> 154 const struct switchdev_obj_mrp *mrp)
> 155 {
> 156 struct ocelot_port *ocelot_port = ocelot->ports[port];
> 157 int i;
> 158
> 159 if (!ocelot_port)
> 160 return -EOPNOTSUPP;
> 161
> 162 if (ocelot_port->mrp_ring_id != mrp->ring_id)
> 163 return 0;
> 164
> 165 ocelot_mrp_del_vcap(ocelot, port);
> 166 ocelot_mrp_del_vcap(ocelot, port + ocelot->num_phys_ports);
> 167
> 168 ocelot_port->mrp_ring_id = 0;
> 169
> 170 for (i = 0; i < ocelot->num_phys_ports; ++i) {
>
> This loop tries to verify that all the ports have ring_id == 0 otherwise
> we return. It's slightly confusing why this matters.
>
> 171 ocelot_port = ocelot->ports[i];
> 172
> 173 if (!ocelot_port)
> ^^^^^^^^^^^
> Assume the last element of the array is NULL
>
> 174 continue;
> 175
> 176 if (ocelot_port->mrp_ring_id != 0)
> 177 goto out;
>
> This would be more clear if instead of a "goto out;" it just did a
> direct "return 0;"
>
> 178 }
> 179
> 180 ocelot_mrp_del_mac(ocelot, ocelot_port);
>
> We delete the last mac address of all the ring_ids are zero but if the
> last port is zero it will crash.
>
> 181 out:
> 182 return 0;
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net: ocelot: Extend MRP
2021-03-18 12:11 ` Dan Carpenter
@ 2021-03-18 18:57 ` Horatiu Vultur
0 siblings, 0 replies; 3+ messages in thread
From: Horatiu Vultur @ 2021-03-18 18:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
The 03/18/2021 15:11, Dan Carpenter wrote:
Hi Dan,
>
> See also:
>
> drivers/net/ethernet/mscc/ocelot_mrp.c:254 ocelot_mrp_del_ring_role() error: we previously assumed 'ocelot_port' could be null (see line 247)
>
> regards,
> dan carpenter
>
> On Thu, Mar 18, 2021 at 03:07:32PM +0300, Dan Carpenter wrote:
> > Hello Horatiu Vultur,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch 7c588c3e96e9: "net: ocelot: Extend MRP" from Mar 16, 2021,
> > leads to the following Smatch complaint:
> >
> > drivers/net/ethernet/mscc/ocelot_mrp.c:180 ocelot_mrp_del()
> > error: we previously assumed 'ocelot_port' could be null (see line 173)
> >
> > drivers/net/ethernet/mscc/ocelot_mrp.c
> > 153 int ocelot_mrp_del(struct ocelot *ocelot, int port,
> > 154 const struct switchdev_obj_mrp *mrp)
> > 155 {
> > 156 struct ocelot_port *ocelot_port = ocelot->ports[port];
> > 157 int i;
> > 158
> > 159 if (!ocelot_port)
> > 160 return -EOPNOTSUPP;
> > 161
> > 162 if (ocelot_port->mrp_ring_id != mrp->ring_id)
> > 163 return 0;
> > 164
> > 165 ocelot_mrp_del_vcap(ocelot, port);
> > 166 ocelot_mrp_del_vcap(ocelot, port + ocelot->num_phys_ports);
> > 167
> > 168 ocelot_port->mrp_ring_id = 0;
> > 169
> > 170 for (i = 0; i < ocelot->num_phys_ports; ++i) {
> >
> > This loop tries to verify that all the ports have ring_id == 0 otherwise
> > we return. It's slightly confusing why this matters.
It is trying to detect if there is any existing MRP ring active on the
node. A port is part of a MRP ring if mrp_ring_id != 0. If there is no
MRP ring then we need to remove the entry in the MAC table otherwise
keep it there.
> >
> > 171 ocelot_port = ocelot->ports[i];
> > 172
> > 173 if (!ocelot_port)
> > ^^^^^^^^^^^
> > Assume the last element of the array is NULL
> >
> > 174 continue;
> > 175
> > 176 if (ocelot_port->mrp_ring_id != 0)
> > 177 goto out;
> >
> > This would be more clear if instead of a "goto out;" it just did a
> > direct "return 0;"
I can change this.
> >
> > 178 }
> > 179
> > 180 ocelot_mrp_del_mac(ocelot, ocelot_port);
> >
> > We delete the last mac address of all the ring_ids are zero but if the
> > last port is zero it will crash.
Yep, it should be ocelot->ports[port] as I have done it in the first
version.
I will send a patch for this.
> >
> > 181 out:
> > 182 return 0;
> >
> > regards,
> > dan carpenter
--
/Horatiu
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-18 18:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-18 12:07 [bug report] net: ocelot: Extend MRP Dan Carpenter
2021-03-18 12:11 ` Dan Carpenter
2021-03-18 18:57 ` Horatiu Vultur
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.