netdev - Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

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:   Wed, 19 Apr 2023 23:14:54 -0300
From:   Pedro Tammela <pctammela@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com
Subject: Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors
 messages

On 19/04/2023 22:08, Jakub Kicinski wrote:
> On Mon, 17 Apr 2023 14:12:15 -0300 Pedro Tammela wrote:
>> @@ -1917,8 +1917,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>   			};
>>   			err = htb_offload(dev, &offload_opt);
>>   			if (err) {
>> -				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
>> -				       err);
>> +				NL_SET_ERR_MSG(extack,
>> +					       "Failed to offload leaf alloc queue");
>>   				goto err_kill_estimator;
>>   			}
>>   			dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
>> @@ -1937,8 +1937,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>   			};
>>   			err = htb_offload(dev, &offload_opt);
>>   			if (err) {
>> -				pr_err("htb: TC_HTB_LEAF_TO_INNER failed with err = %d\n",
>> -				       err);
>> +				NL_SET_ERR_MSG(extack,
>> +					       "Failed to offload leaf to inner");
> 
> I missed the message changes on v1, but since the patches are already
> Changes Requested in patchwork...
> 
> IDK what TC_HTB_LEAF_TO_INNER is exactly, neither do I understand
> "Failed to offload leaf to inner". The first one should be "Failed to
> alloc offload leaf queue" if anything.
> 
> Let's just stick to the existing messages?

The existing messages omit that the offload operation failed, which I 
think it would be important to say in these two cases.
But taking a second look, the driver gets the extack so it should be up 
to the driver to provide the appropriate context when the offload fails. 
What do you think about demoting these messages to WEAK and change them 
to something like:
"Failed to offload %attribute"