netdev - Re: [PATCH net-next v3 3/3] virtio-net: synchronize operstate with admin state on up/down

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240718211816-mutt-send-email-mst@kernel.org>
Date: Thu, 18 Jul 2024 21:19:41 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	Venkat Venkatsubra <venkat.x.venkatsubra@...cle.com>,
	Gia-Khanh Nguyen <gia-khanh.nguyen@...cle.com>
Subject: Re: [PATCH net-next v3 3/3] virtio-net: synchronize operstate with
 admin state on up/down

On Fri, Jul 19, 2024 at 09:02:29AM +0800, Jason Wang wrote:
> On Wed, Jul 17, 2024 at 2:53 PM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Wed, Jul 17, 2024 at 2:00 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > >
> > > On Wed, Jul 17, 2024 at 09:19:02AM +0800, Jason Wang wrote:
> > > > On Wed, Jul 10, 2024 at 11:03 AM Jason Wang <jasowang@...hat.com> wrote:
> > > > >
> > > > > On Tue, Jul 9, 2024 at 9:28 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 09, 2024 at 04:02:14PM +0800, Jason Wang wrote:
> > > > > > > This patch synchronize operstate with admin state per RFC2863.
> > > > > > >
> > > > > > > This is done by trying to toggle the carrier upon open/close and
> > > > > > > synchronize with the config change work. This allows propagate status
> > > > > > > correctly to stacked devices like:
> > > > > > >
> > > > > > > ip link add link enp0s3 macvlan0 type macvlan
> > > > > > > ip link set link enp0s3 down
> > > > > > > ip link show
> > > > > > >
> > > > > > > Before this patch:
> > > > > > >
> > > > > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > > > > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > > > > ......
> > > > > > > 5: macvlan0@...0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > > > > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > > > > >
> > > > > > > After this patch:
> > > > > > >
> > > > > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > > > > >     link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > > > > ...
> > > > > > > 5: macvlan0@...0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > > > > >     link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > > > >
> > > > > > I think that the commit log is confusing. It seems to say that
> > > > > > the issue fixed is synchronizing state with hardware
> > > > > > config change.
> > > > > > But your example does not show any
> > > > > > hardware change. Isn't this example really just
> > > > > > a side effect of setting carrier off on close?
> > > > >
> > > > > The main goal for this patch is to make virtio-net follow RFC2863. The
> > > > > main thing that is missed is to synchronize the operstate with admin
> > > > > state, if we do this, we get several good results, one of the obvious
> > > > > one is to allow virtio-net to propagate status to the upper layer, for
> > > > > example if the admin state of the lower virtio-net is down it should
> > > > > be propagated to the macvlan on top, so I give the example of using a
> > > > > stacked device. I'm not we had others but the commit log is probably
> > > > > too small to say all of it.
> > > >
> > > > Michael, any more comments on this?
> > > >
> > > > Thans
> > >
> > >
> > > Still don't get it, sorry.
> > > > > > > This is done by trying to toggle the carrier upon open/close and
> > > > > > > synchronize with the config change work.
> > > What does this sentence mean? What is not synchronized with config
> > > change that needs to be?
> >
> > I meant,
> >
> > 1) maclvan depends on the linkwatch to transfer operstate from the
> > lower device to itself.
> > 2) ndo_open()/close() will not trigger the linkwatch so we need to do
> > it by ourselves in virtio-net to make sure macvlan get the correct
> > opersate
> > 3) consider config change work can change the state so ndo_close()
> > needs to synchronize with it
> >
> > Thanks
> 
> Michael, are you fine with the above or I miss something there?
> 
> Thanks


I don't understand 3. config change can always trigger.
what I do not like is all these reads from config space
that now trigger on open/close. previously we did
read
- on probe
- after probe, if config changed


and that made sense.

-- 
MST