linux-kernel - RE: [PATCH v2 1/2] virtio-balloon: tweak config

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]

Date:   Sat, 5 Jan 2019 03:32:44 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
CC:     "virtio-dev@...ts.oasis-open.org" <virtio-dev@...ts.oasis-open.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "pasic@...ux.ibm.com" <pasic@...ux.ibm.com>,
        "borntraeger@...ibm.com" <borntraeger@...ibm.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "dgilbert@...hat.com" <dgilbert@...hat.com>
Subject: RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed
 implementation

On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> >  struct virtio_balloon {
> >  	struct virtio_device *vdev;
> >  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ struct virtio_balloon {
> >  	/* Prevent updating balloon when it is being canceled. */
> >  	spinlock_t stop_update_lock;
> >  	bool stop_update;
> > +	/* Bitmap to indicate if reading the related config fields are needed
> */
> > +	unsigned long config_read_bitmap;
> >
> >  	/* The list of allocated free pages, waiting to be given back to mm */
> >  	struct list_head free_page_list;
> 
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.

We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)

> > +
> >  static int send_free_pages(struct virtio_balloon *vb)  {
> >  	int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> >  		 * stop the reporting.
> >  		 */
> >  		cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > +		virtio_balloon_read_cmd_id_received(vb);
 

[1]


> >  		if (cmd_id_active != vb->cmd_id_received)
> >  			break;
> >
> > @@ -637,11 +648,9 @@ static int send_free_pages(struct virtio_balloon
> *vb)
> >  	return 0;
> >  }
> >
> > -static void report_free_page_func(struct work_struct *work)
> > +static void virtio_balloon_report_free_page(struct virtio_balloon
> > +*vb)
> >  {
> >  	int err;
> > -	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > -						 report_free_page_work);
> >  	struct device *dev = &vb->vdev->dev;
> >
> >  	/* Start by sending the received cmd id to host with an outbuf. */
> > @@ -659,6 +668,22 @@ static void report_free_page_func(struct
> work_struct *work)
> >  		dev_err(dev, "Failed to send a stop id, err = %d\n", err);  }
> >
> > +static void report_free_page_func(struct work_struct *work) {
> > +	struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > +						 report_free_page_work);
> > +
> > +	virtio_balloon_read_cmd_id_received(vb);
> 
> This will not achieve what you are trying to do, which is cancel reporting if it's
> in progress.
> 
> You need to re-read each time you compare to cmd_id_active.

Yes, already did, please see [1] above


> An API similar to
> 	u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
> 

I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called "cmd_id_active" which
is the one that the driver is actively using. 
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.

Best,
Wei