netdev - RE: [net-next v2 07/11] devlink: report error once U32

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:   Thu, 26 Mar 2020 16:09:44 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: RE: [net-next v2 07/11] devlink: report error once U32_MAX snapshot
 ids have been used



> -----Original Message-----
> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> Behalf Of Jiri Pirko
> Sent: Thursday, March 26, 2020 12:31 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: netdev@...r.kernel.org; Jakub Kicinski <kuba@...nel.org>
> Subject: Re: [net-next v2 07/11] devlink: report error once U32_MAX snapshot
> ids have been used
> 
> Thu, Mar 26, 2020 at 04:51:53AM CET, jacob.e.keller@...el.com wrote:
> >The devlink_snapshot_id_get() function returns a snapshot id. The
> >snapshot id is a u32, so there is no way to indicate an error code.
> >
> >Indeed, the two current callers of devlink_snapshot_id_get() assume that
> >a negative value is an error.
> 
> I don't see they do.
> 

You're right, they don't. I was confused when I wrote this because the previous version of this patch did change them to check for error, and then I realized we didn't need that patch and removed it, but forgot to update this.

Can fix this if we need a respin for another reason.

> 
> >
> >A future change is going to possibly add additional cases where this
> >function could fail. Refactor the function to return the snapshot id in
> >an argument, so that it can return zero or an error value.
> >
> >This ensures that snapshot ids cannot be confused with error values, and
> >aids in the future refactor of snapshot id allocation management.
> >
> >Because there is no current way to release previously used snapshot ids,
> >add a simple check ensuring that an error is reported in case the
> >snapshot_id would over flow.
> >
> >Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> 
> The code looks good.
> 
> Reviewed-by: Jiri Pirko <jiri@...lanox.com>