"panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

[Mateusz Guzik]

On 8/18/20, Mark Johnston <markj at freebsd.org> wrote:
> On Tue, Aug 18, 2020 at 03:24:52PM +0200, Mateusz Guzik wrote:
>> Try this:
>>
>> diff --git a/sys/kern/kern_malloc.c b/sys/kern/kern_malloc.c
>> index 46eb1c4347c..7b94ca7b880 100644
>> --- a/sys/kern/kern_malloc.c
>> +++ b/sys/kern/kern_malloc.c
>> @@ -618,8 +618,8 @@ void *
>>         unsigned long osize = size;
>>  #endif
>>
>> -       KASSERT((flags & M_WAITOK) == 0 || THREAD_CAN_SLEEP(),
>> -           ("malloc(M_WAITOK) in non-sleepable context"));
>> +       if ((flags & M_WAITOK) != 0)
>> +               WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
>> __func__);
>>
>>  #ifdef MALLOC_DEBUG
>>         va = NULL;
>> diff --git a/sys/vm/uma_core.c b/sys/vm/uma_core.c
>> index 37d78354200..2e1267ec02f 100644
>> --- a/sys/vm/uma_core.c
>> +++ b/sys/vm/uma_core.c
>> @@ -3355,8 +3355,8 @@ uma_zalloc_arg(uma_zone_t zone, void *udata, int
>> flags)
>>         uma_cache_bucket_t bucket;
>>         uma_cache_t cache;
>>
>> -       KASSERT((flags & M_WAITOK) == 0 || THREAD_CAN_SLEEP(),
>> -           ("uma_zalloc(M_WAITOK) in non-sleepable context"));
>> +       if ((flags & M_WAITOK) != 0)
>> +               WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
>> __func__);
>>
>>         /* Enable entropy collection for RANDOM_ENABLE_UMA kernel option
>> */
>>         random_harvest_fast_uma(&zone, sizeof(zone), RANDOM_UMA);
>
> Doesn't it only print a warning if non-sleepable locks are held?
> THREAD_CAN_SLEEP() catches other cases, like epoch sections and worker
> threads that are not allowed to sleep (CAM doneq threads in this case).
> Otherwise uma_zalloc_debug() already checks with WITNESS.
>
> I'm inclined to simply revert the commit for now.  Alternately,
> disk_alloc() could be modified to handle M_NOWAIT/M_WAITOK flags, and
> that could be used in daregister() and other CAM periph drivers.
> daregister() already uses M_NOWAIT to allocate the driver softc itself.
>

If WITNESS_WARN(.., WARN_SLEEPOK) does not check for all possible
blockers for going off CPU that's a bug. I do support reverting the
patch for now until the dust settles. I don't propose the above hack
as the final fix.

As for the culrpit at hand, given the backtrace:
devfs_alloc() at devfs_alloc+0x28/frame 0xffffffff8218d570
make_dev_sv() at make_dev_sv+0x97/frame 0xffffffff8218d600
make_dev_s() at make_dev_s+0x3b/frame 0xffffffff8218d660
passregister() at passregister+0x3e7/frame 0xffffffff8218d8b0

I think this is missing wait flags, resulting in M_WAITOK later, i.e.:
diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c
index b299fbddd84..c6d6a5da403 100644
--- a/sys/cam/scsi/scsi_pass.c
+++ b/sys/cam/scsi/scsi_pass.c
@@ -652,6 +652,7 @@ passregister(struct cam_periph *periph, void *arg)
        args.mda_gid = GID_OPERATOR;
        args.mda_mode = 0600;
        args.mda_si_drv1 = periph;
+       args.mda_flags = MAKEDEV_NOWAIT;
        error = make_dev_s(&args, &softc->dev, "%s%d", periph->periph_name,
            periph->unit_number);
        if (error != 0) {


> diff --git a/sys/cam/scsi/scsi_da.c b/sys/cam/scsi/scsi_da.c
> index d648d511b1a2..d940e7db24e0 100644
> --- a/sys/cam/scsi/scsi_da.c
> +++ b/sys/cam/scsi/scsi_da.c
> @@ -2767,18 +2767,23 @@ daregister(struct cam_periph *periph, void *arg)
>  		return(CAM_REQ_CMP_ERR);
>  	}
>
> -	softc = (struct da_softc *)malloc(sizeof(*softc), M_DEVBUF,
> -	    M_NOWAIT|M_ZERO);
> -
> +	softc = malloc(sizeof(*softc), M_DEVBUF, M_NOWAIT | M_ZERO);
>  	if (softc == NULL) {
>  		printf("daregister: Unable to probe new device. "
>  		       "Unable to allocate softc\n");
>  		return(CAM_REQ_CMP_ERR);
>  	}
> +	softc->disk = disk_alloc_flags(M_NOWAIT);
> +	if (softc->disk == NULL) {
> +		printf("daregister: Unable to probe new device. "
> +		       "Unable to allocate disk structure\n");
> +		return (CAM_REQ_CMP_ERR);
> +	}
>
>  	if (cam_iosched_init(&softc->cam_iosched, periph) != 0) {
>  		printf("daregister: Unable to probe new device. "
>  		       "Unable to allocate iosched memory\n");
> +		disk_destroy(softc->disk);
>  		free(softc, M_DEVBUF);
>  		return(CAM_REQ_CMP_ERR);
>  	}
> @@ -2898,7 +2903,6 @@ daregister(struct cam_periph *periph, void *arg)
>  	/*
>  	 * Register this media as a disk.
>  	 */
> -	softc->disk = disk_alloc();
>  	softc->disk->d_devstat = devstat_new_entry(periph->periph_name,
>  			  periph->unit_number, 0,
>  			  DEVSTAT_BS_UNAVAILABLE,
> diff --git a/sys/geom/geom_disk.c b/sys/geom/geom_disk.c
> index eaba770828d0..c07494ab4ec0 100644
> --- a/sys/geom/geom_disk.c
> +++ b/sys/geom/geom_disk.c
> @@ -850,15 +850,22 @@ g_disk_ident_adjust(char *ident, size_t size)
>  }
>
>  struct disk *
> -disk_alloc(void)
> +disk_alloc_flags(int mflag)
>  {
>  	struct disk *dp;
>
> -	dp = g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO);
> -	LIST_INIT(&dp->d_aliases);
> +	dp = g_malloc(sizeof(struct disk), mflag | M_ZERO);
> +	if (dp != NULL)
> +		LIST_INIT(&dp->d_aliases);
>  	return (dp);
>  }
>
> +struct disk *
> +disk_alloc(void)
> +{
> +	return (disk_alloc_flags(M_WAITOK));
> +}
> +
>  void
>  disk_create(struct disk *dp, int version)
>  {
> diff --git a/sys/geom/geom_disk.h b/sys/geom/geom_disk.h
> index 8e2282a09a3a..794ce2cc38bd 100644
> --- a/sys/geom/geom_disk.h
> +++ b/sys/geom/geom_disk.h
> @@ -137,6 +137,7 @@ struct disk {
>  #define	DISKFLAG_WRITE_PROTECT		0x0100
>
>  struct disk *disk_alloc(void);
> +struct disk *disk_alloc_flags(int mflag);
>  void disk_create(struct disk *disk, int version);
>  void disk_destroy(struct disk *disk);
>  void disk_gone(struct disk *disk);
>


-- 
Mateusz Guzik <mjguzik gmail.com>