[bug report] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl

[bug report] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl

[Dan Carpenter]

Hello Rick Edgecombe,

Commit 211f514ebf1e ("Drivers: hv: vmbus: Track decrypted status in
vmbus_gpadl") from Mar 11, 2024 (linux-next), leads to the following
Smatch static checker warning:

	drivers/hv/channel.c:870 vmbus_teardown_gpadl()
	warn: assigning signed to unsigned: 'gpadl->decrypted = ret' 's32min-s32max'

drivers/hv/channel.c
    860         list_del(&info->msglistentry);
    861         spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
    862 
    863         kfree(info);
    864 
    865         ret = set_memory_encrypted((unsigned long)gpadl->buffer,
    866                                    PFN_UP(gpadl->size));
    867         if (ret)
    868                 pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
    869 
--> 870         gpadl->decrypted = ret;

ret is error codes but ->decrypted is bool.  So error codes mean decrypted is
true.

    871 
    872         return ret;
    873 }

regards,
dan carpenter

Re: [bug report] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl

[Edgecombe, Rick P]

On Sat, 2024-07-27 at 00:33 -0500, Dan Carpenter wrote:
> Commit 211f514ebf1e ("Drivers: hv: vmbus: Track decrypted status in
> vmbus_gpadl") from Mar 11, 2024 (linux-next), leads to the following
> Smatch static checker warning:
> 
>         drivers/hv/channel.c:870 vmbus_teardown_gpadl()
>         warn: assigning signed to unsigned: 'gpadl->decrypted = ret' 's32min-
> s32max'
> 
> drivers/hv/channel.c
>     860         list_del(&info->msglistentry);
>     861         spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,
> flags);
>     862 
>     863         kfree(info);
>     864 
>     865         ret = set_memory_encrypted((unsigned long)gpadl->buffer,
>     866                                    PFN_UP(gpadl->size));
>     867         if (ret)
>     868                 pr_warn("Fail to set mem host visibility in GPADL
> teardown %d.\n", ret);
>     869 
> --> 870         gpadl->decrypted = ret;
> 
> ret is error codes but ->decrypted is bool.  So error codes mean decrypted is
> true.

If it fails, we need to assume that some of the buffer could still be decrypted,
so should have decrypted = true. Only if it is successful (ret == 0) should we
have decrypted = false.

So I think it is functionally correct. Should we have a cast for smatch's sake?
I would have thought there would be a lot of these patterns in the kernel.

If we unconditionally set decrypted = false here, I think we would be ok as
well. That would match the other similar cases better.

Re: [bug report] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl

[Dan Carpenter]

On Mon, Jul 29, 2024 at 05:13:56PM +0000, Edgecombe, Rick P wrote:
> On Sat, 2024-07-27 at 00:33 -0500, Dan Carpenter wrote:
> > Commit 211f514ebf1e ("Drivers: hv: vmbus: Track decrypted status in
> > vmbus_gpadl") from Mar 11, 2024 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> >         drivers/hv/channel.c:870 vmbus_teardown_gpadl()
> >         warn: assigning signed to unsigned: 'gpadl->decrypted = ret' 's32min-
> > s32max'
> > 
> > drivers/hv/channel.c
> >     860         list_del(&info->msglistentry);
> >     861         spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,
> > flags);
> >     862 
> >     863         kfree(info);
> >     864 
> >     865         ret = set_memory_encrypted((unsigned long)gpadl->buffer,
> >     866                                    PFN_UP(gpadl->size));
> >     867         if (ret)
> >     868                 pr_warn("Fail to set mem host visibility in GPADL
> > teardown %d.\n", ret);
> >     869 
> > --> 870         gpadl->decrypted = ret;
> > 
> > ret is error codes but ->decrypted is bool.  So error codes mean decrypted is
> > true.
> 
> If it fails, we need to assume that some of the buffer could still be decrypted,
> so should have decrypted = true. Only if it is successful (ret == 0) should we
> have decrypted = false.
> 
> So I think it is functionally correct. Should we have a cast for smatch's sake?

Thanks for looking at this.

Generally the rule is to not do anything just make the checker happy.
These are one time warnings.  Kernel devs are really good at fixing
bugs so old warnings are all false positives.  Plus this thread is there
on lore if people have questions about it.

This isn't a published check yet, but I think I'm going to publish
something else which prints a warning here.  This check warns if there
is a known negative value assigned to an unsigned, but I'm going to
write a check which complains about negative error codes assigned to
unsigned types smaller than int.

> I would have thought there would be a lot of these patterns in the kernel.
> 

It's not super common.  The most common place to see this is when
functions return false on success and true on failure.  I don't know
why people do that...  :/

regards,
dan carpenter