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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250221095719.11661Ba2-hca@linux.ibm.com>
Date: Fri, 21 Feb 2025 10:57:19 +0100
From: Heiko Carstens <hca@...ux.ibm.com>
To: Anthony Krowiak <akrowiak@...ux.ibm.com>
Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, jjherne@...ux.ibm.com, borntraeger@...ibm.com,
        pasic@...ux.ibm.com, pbonzini@...hat.com, frankja@...ux.ibm.com,
        imbrenda@...ux.ibm.com, alex.williamson@...hat.com,
        kwankhede@...dia.com, agordeev@...ux.ibm.com, gor@...ux.ibm.com
Subject: Re: [PATCH] s390/vio-ap: Fix no AP queue sharing allowed message
 written to kernel log

On Wed, Feb 19, 2025 at 07:07:38PM -0500, Anthony Krowiak wrote:
> -#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \
> -			 "already assigned to %s"
> +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " \
> +			 "to mdev: already assigned to %s"

Please do not split error messages across several lines, so it is easy
to grep such for messages. If this would have been used for printk
directly checkpatch would have emitted a message.

> +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host driver: " \
> +			"in use by mdev"

Same here.

>  	for_each_set_bit_inv(apid, apm, AP_DEVICES)
>  		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
> -			dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name);
> +			dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR,
> +				 apid, apqi, dev_name(mdev_dev(assigned_to->mdev)));

Braces are missing. Even it the above is not a bug: bodies of for
statements must be enclosed with braces if they have more than one
line:

  	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
  		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
			dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR,
				 apid, apqi, dev_name(mdev_dev(assigned_to->mdev))
		}
	}

> +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev *assignee,
> +					unsigned long *apm, unsigned long *aqm)
> +{
> +	unsigned long apid, apqi;
> +
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES)
> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
> +			dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR,
> +				 apid, apqi);
> +}

Same here.

> +
> +/**assigned
>   * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs

Stray "assigned" - as a result this is not kernel doc anymore.

> + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being
> + *           assigned; or, NULL if this function was called by the AP bus driver
> + *           in_use callback to verify none of the APQNs being reserved for the
> + *           host device driver are in use by a vfio_ap mediated device
>   * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>   * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified

Missing ":" behind @assignee. Please keep this consistent.

> @@ -912,17 +930,21 @@ static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
>  
>  		/*
>  		 * We work on full longs, as we can only exclude the leftover
> -		 * bits in non-inverse order. The leftover is all zeros.
> +		 * bits in non-inverse order. The leftover is all zeros.assigned
>  		 */

Another random "assigned" word.

> +		if (assignee)
> +			vfio_ap_mdev_log_sharing_err(assignee, assigned_to,
> +						     apm, aqm);
> +		else
> +			vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm);

if body with multiple lines -> braces. Or better make that
vfio_ap_mdev_log_sharing_err() call a long line. If you want to keep
the line-break add braces to both the if and else branch.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ