Hi Heiko,
On Mon, Mar 22, 2021 at 11:57:33AM +0100, Heiko Thiery wrote:
> Hi Vladimir and all,
>
> > Due to the fact that ic_dev->dev is kept open in ic_close_dev, I had
> > thought that ic_dev will not be freed either. But that is not the case,
> > but instead "everybody dies" when ipconfig cleans up, and just the
> > net_device behind ic_dev->dev remains allocated but not ic_dev itself.
> >
> > This is a problem because in ic_close_devs, for every net device that
> > we're about to close, we compare it against the list of lower interfaces
> > of ic_dev, to figure out whether we should close it or not. But since
> > ic_dev itself is subject to freeing, this means that at some point in
> > the middle of the list of ipconfig interfaces, ic_dev will have been
> > freed, and we would be still attempting to iterate through its list of
> > lower interfaces while checking whether to bring down the remaining
> > ipconfig interfaces.
> >
> > There are multiple ways to avoid the use-after-free: we could delay
> > freeing ic_dev until the very end (outside the while loop). Or an even
> > simpler one: we can observe that we don't need ic_dev when iterating
> > through its lowers, only ic_dev->dev, structure which isn't ever freed.
> > So, by keeping ic_dev->dev in a variable assigned prior to freeing
> > ic_dev, we can avoid all use-after-free issues.
> >
> > Fixes: 46acf7bdbc72 ("Revert "net: ipv4: handle DSA enabled master network
> > devices"")
> > Reported-by: kernel test robot <oliver.s...@intel.com>
> > Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> > ---
> > net/ipv4/ipconfig.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> > index f9ab1fb219ec..47db1bfdaaa0 100644
> > --- a/net/ipv4/ipconfig.c
> > +++ b/net/ipv4/ipconfig.c
> > @@ -309,6 +309,7 @@ static int __init ic_open_devs(void)
> > */
> > static void __init ic_close_devs(void)
> > {
> > + struct net_device *selected_dev = ic_dev->dev;
>
> This will causes a kernel panic when ic_dev is not valid.
>
> See log output here: https://lavalab.kontron.com/scheduler/job/12453
>
> > struct ic_device *d, *next;
> > struct net_device *dev;
> >
> > @@ -322,7 +323,7 @@ static void __init ic_close_devs(void)
> > next = d->next;
> > dev = d->dev;
> >
> > - netdev_for_each_lower_dev(ic_dev->dev, lower_dev, iter) {
> > + netdev_for_each_lower_dev(selected_dev, lower_dev, iter) {
> > if (dev == lower_dev) {
> > bring_down = false;
> > break;
> > --
> > 2.25.1
>
>
> I can fix my issue with this:
>
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index 47db1bfdaaa0..6f1df4336153 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -309,7 +309,6 @@ static int __init ic_open_devs(void)
> */
> static void __init ic_close_devs(void)
> {
> - struct net_device *selected_dev = ic_dev->dev;
> struct ic_device *d, *next;
> struct net_device *dev;
>
> @@ -323,10 +322,13 @@ static void __init ic_close_devs(void)
> next = d->next;
> dev = d->dev;
>
> - netdev_for_each_lower_dev(selected_dev, lower_dev, iter) {
> - if (dev == lower_dev) {
> - bring_down = false;
> - break;
> + if (ic_dev) {
> + struct net_device *selected_dev = ic_dev->dev;
> + netdev_for_each_lower_dev(selected_dev, lower_dev,
> iter) {
> + if (dev == lower_dev) {
> + bring_down = false;
> + break;
> + }
> }
> }
> if (bring_down) {
>
>
> What do you think? Is this valid?
>
> Thank you.
> --
> Heiko
I guess you're the right person to give me a Reviewed-by/Tested-by here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210322002637.3412657-1-olte...@gmail.com/
