这个修改居然是二掌门的主意

最近在做一个反向移植工作，把Linux 6.1的OPP模块移植到5.10上。做这个移植首先是为了解决GPU有关的一个问题，同时也是想试验一下局部升级内核的方法。

对于这样的移植任务，主要的工作就是适配各种变化了的接口。有些是数据结构的变化，有些是函数原型的变化。

对于大多数软件工程师来说，都接受模块化的设计思想，让代码以模块为单位聚拢，模块内部的代码尽可能紧凑，模块之间的接口尽量保持稳定。这样内部可以不断改进，而又不影响外部。但是这个道理在一些Linux内核的开发者那里似乎说不通，他们在做修改时，常常做各种破坏性的修改。改结构体时，把新加的字段不是加在兼容性更好的末尾，而是加在破坏兼容的中间位置。对于接口函数，也常常改来改去，有时改名字，有时加减参数，有时不加不减，只是把顺序调一调。

砍掉的参数

比如，在这次移植OPP时，就因为一个函数的原型修改而引入了一大堆错误。

老的函数原型为：

struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
const char * const names[], unsigned int count);    

新的函数原型为：

struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
const char * const names[]);    

比较新旧两个函数，很容易就能看出是少了一个参数，本来是三个参数，改了之后是两个参数。

从函数名来看，这是个接口函数，具有很正式的名字，以dev_pm_opp开头，代表模块名，整个函数名是“主谓宾”结构，含义清楚，名字的长度也还可以。

新旧函数的前两个参数是一样的，第一个是设备对象，第二个参数是字符串数组。

对于老的写法，第三个参数是数组中元素的个数。

在我看来，老的函数原型很好，每个参数都含义明确，让人一看便知用法。

可是就这么一个看似完美的函数，却在新版本中被活生生改掉了。本来不多不少的三个参数硬是被砍掉了一个，变成2个。

而且砍掉的一个是在我看来很重要的数组元素个数。大家都知道，数组本身是无法表示元素个数的，因此使用第三个参数明确传递元素个数，是合理的，而且也是必要的。

可是，这个元素个数的参数就是被砍掉了。

有一种教育方法

叫抓“现形” 

BETRAY

我不喜欢新的写法，因此在处理这个变化时，选择了老的方法，保持三个参数，调用的地方不用改，只是把函数实现的方法，把老的count参数加回来，和新代码对接一下。

到中午时，一起做移植的小伙伴吃饭去了，我多工作一会儿，把这个问题基本修改好之后才吃中饭。

待我吃好中饭休息了一下回到座位时，小伙伴已经开工一会了，我看了一下代码，吃惊地发现，他用了另外一种改法，使用新的两个参数写法，把所有调用的地方都去掉了一个参数。

我看了后，哭笑不得。我改时，他去吃中饭，我没有和他说清楚，所以也不能怪他。而且事情已经发生了，后悔也没有用。

但是本着“教育”的目的，我和他说，老的写法好一些，明确表达数组个数，更加安全。

其实我还想说更多，因为在我看来，使用新的写法除了去掉参数外，还有其它工作要做，不然可能会出问题的。但是我把后面的话咽了下去，没有说出口。

对于已经说的，我不确认小伙伴是否听进去了。这么多年的讲课经验告诉我，思想传递是个复杂的过程，一个人说的话，另一个人能听进去几分，只有上帝知道。涉及到“教育”的更是如此，大多数人的思想都是有“防入侵”功能的，对于别人的说教，第一反应常常就是防卫。况且，现在是工作场景，我虽然年龄大一些，但是彼此也是同事关系。

这个问题就这样暂时过去，开始修改其它问题。其它问题都比较好改，于是新的内核在一个小时后就编译出来了，下一步是刷机验证。

刷机后，果然如我预料，出问题了，系统panic。

看调用栈，就是上面提到的函数dev_pm_opp_set_regulators。

顺着调用栈往上看子函数，上面几层都和字符串有关。因为这个函数的第二个参数就是一个字符串数组。Panic开头的“判决”信息是访问了无效指针：

[    7.868736] Unable to handle kernel paging request at virtual address 02ed7eddcd273200
[    7.869534] Mem abort info:
[    7.869864]   ESR = 0x96000004
[    7.870184]   EC = 0x25: DABT (current EL), IL = 32 bits
[    7.870712]   SET = 0, FnV = 0
[    7.871028]   EA = 0, S1PTW = 0

根据我的经验，越界了，因为新的函数原型去掉了元素个数参数，所以循环遍历数组时，越界了，把内存里的这个数据02ed7eddcd273200当作指针了。

以NULL结束

真的好么？ 

NULL

读到这里，可能有人憋不住了，新的写法怎么表达数组元素个数呢？

答案是以一个空的元素表示结尾。

在新代码的注释里，说明了这一点：

 * @regulator_names: Array of pointers to the names of the regulator, NULL terminated.

在新代码的内部实现里，有个循环来数数组的元素个数，即：

static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
                   const char * const names[])
{
    const char * const *temp = names;
    struct regulator *reg;
    int count = 0, ret, i;

    /* Count number of regulators */
    while (*temp++)
        count++;

这意味着，使用新的函数后，所有调用的地方，都必须保证数组的末尾有个NULL元素。

如果老的写法是这样：

static const char * const omap3_reg_names[] = {"cpu0", "vbb"};

那么必须改成下面这样：

static const char * const omap3_reg_names[] = {"cpu0", "vbb", NULL};

对于上面这样的代码，改起来比较容易。

对于本来就是动态数组的情况（如果它本来没有以NULL结束），改起来就要麻烦一些。

比如对于上面的情况，如果本来的supply_names数组不是以NULL结尾，那么就需要复制到一个新数组，再加上一个NULL结束。

从安全角度看

SECURE

从代码安全的角度看，新的写法很不好，依靠数组的一个NULL元素来表达数组结束，这意味着凭借数据区里的一个特殊值来结束循环。

大家都知道，内存里的数据部分是比较容易被攻击的，或者说数据区的NULL标志是容易被黑客通过内存溢出等手段篡改的。而这个标志一旦被篡改，那么便有可能出很多问题了，比如无限循环，访问意外数据等…….

相对而言，使用固定一个参数“显式”表达数组长度是更安全的。如果参数是立即数，那么它一般是被编码到代码空间的，代码空间是只读的，具有更好的安全性。

其实，很多安全问题就是因为隐式信息传递而产生的。

谁干的？

WHO

对于这个既折腾人、又降低安全性的修改是谁做的呢？

下班到家，吃过晚饭，我想查个究竟。打开github上的Linux代码仓库，找到包含上面函数的源文件core.c，然后点击Blame按钮。

我很喜欢这个Blame功能，它就是用来监督代码质量的，让大家可以追查坏代码的源头。

找到被砍参数的内部函数，在左侧边栏看修改人和修改记录。

修改比较多，查看最近的一次修改，不是要找的。因为在它修改前，count参数就已经被砍掉了，顺着时间往前找，有点不好找。

正在这时，我看到左侧修改记录里有一句话：

OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list

从标题看就是它，看来为了这事，还有一个专门的整改和补丁。详细的记录如下：

https://github.com/torvalds/linux/commit/87686cc845c3be7dea777f1dbf2de0767007cda8

OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list
Make dev_pm_opp_set_regulators() accept a NULL terminated list of names
instead of making the callers keep the two parameters in sync, which
creates an opportunity for bugs to get in.

Suggested-by: Greg Kroah-Hartman <[e-mail]>
Reviewed-by: Steven Price <[e-mail]> # panfrost
Reviewed-by: Chanwoo Choi <[e-mail]>
Signed-off-by: Viresh Kumar <[e-mail]>

• master v6.4-rc4

• v6.0-rc1

vireshk committed on Jul 8, 2022

这个修改的提交时间是2022年7月8日，提交人是vireshk，来自印度（Bangalore, India）。

虽然提交人来自印度，但是大家不要立刻就把锅扣到印度同行的头上。因为修改记录里明白地写着：

Suggested-by: Greg Kroah-Hartman

意思是有人建议这样做的。谁建议的？我肯定不会这么建议，我建议了，印度人很可能也不会听（^_^）。

看到这个建议人的名字，我好惊诧，这可不是一般人，他是Linux基金会的少数几个院士之一。抛开那些挂名的院士之外，Linux基金会的真正院士主要有两位，一位是内核创始人林纳斯，另一位就是这个葛雷格·克罗哈曼，简称GregKH。

葛雷格负责Linux内核的stable分支，以及驱动（drivers）部分。不要小看驱动，从代码量来看，驱动是Linux内核的最大一块。从产业来讲，驱动也是实实在在影响产品研发进度的。

Gregg与林纳斯

葛雷格的院士头衔可不是虚的，他对Linux内核的贡献可谓巨大，在内核代码树里搜索他的名字，可以搜到大量代码。

他也是很多模块的维护者，把握着这些模块的技术方向，守卫着这些模块的质量。

特别值得一提的是，2018年，林纳斯大神赌气休假期间，正是这位葛雷格临时接替林纳斯的角色，担任内核的总舵手。因为此，把他称为Linux内核的“二号掌门人”一点也不为过。

在上面提到的修改说明中，特别提到修改原因是：让dev_pm_opp_set_regulators() 函数接受NULL结束的名字列表，以避免让调用者传递两个需要保持同步的参数（instead of making the callers keep the two parameters in sync）, 那样会让bug有机会进来（which creates an opportunity for bugs to get in.）

其实，两个参数相互验证是有助于提高安全性的。如果说为了防止调用者多传个参数引入bug，那么改了之后，调用者可能忘了以NULL结束，那么引入的就不是一个小bug，而是一个安全漏洞。

对于葛雷格这样一位手刃代码无数的大师提出上面这样的一个修改建议，真是值得思考。是职位高了，技术生疏了？是智者千虑，偶有一失？还是反复思考，故意为之？ 

或许是后者，因为对于如何写代码这件事，大家的观点确实常常有很大的分歧。不知各位格友如何看待这个问题？

我心中隐约有个预兆，说一定哪一天，那个count参数又被加回来了，那时很多代码又要跟着改一遍。

- END -

购买幽兰代码本即可成为兰舍会员，与众多技术高手一起成长。

购买可前往淘宝格友小店：

https://m.tb.cn/h.Uuv7fit?tk=N1iIdn8t4CI

盛格塾是格蠹科技旗下的知识分享平台，是以“格物致知”为教育理念的现代私塾。

本着为先圣继绝学的思想，盛格塾努力将传统文化中的精华与现代科技密切结合，以传统文化和人文情怀阐释现代科技，用现代科技传播传统文化。

访问方式

手机端：微信小程序搜索“盛格塾”

电脑端：下载Nano Code社区版客户端

https://nanocode.cn/#/download

格友公众号

盛格塾小程序

往期 · 精彩推荐